[API Overuse Study] Mock endpoint that simulates a quota aware API #27158
Conversation
- Update README with pipeline review - Configure service and model proto definitions - Configure buf.build for proto generation - Autogenerate go and java proto stubs - Implement gRPC services and cache - Implement Kubernetes client - Configure Infrastructure-as-Code for Google Cloud resources - Configure Infrastructure-as-Code for Kubernetes resources
|
@jrmccluskey Thank you for reviewing! |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @damccorm for label build. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
damondouglas
changed the title
| // Variable's string value assigns the system environment variable key. | ||
| type Variable string | ||
|
|
||
| // Default a value to the system environment. |
There was a problem hiding this comment.
This comment isn't clear for what the method is doing
There was a problem hiding this comment.
The method name doesn't seem to match the implementation either, since the function only sets the default IFF the variable doesn't already have a value.
| // Key returns the system environment variable key. | ||
| func (v Variable) Key() string { | ||
| return (string)(v) | ||
| } | ||
|
|
||
| // Value returns the system environment variable value. | ||
| func (v Variable) Value() string { | ||
| return os.Getenv((string)(v)) | ||
| } |
There was a problem hiding this comment.
Nit: The Key-Value naming here isn't inaccurate, but it feels somewhat strained since the Variable type itself only encapsulates the string name of the environment variable.
| // Default a value to the system environment. | ||
| func (v Variable) Default(value string) error { | ||
| if v.Missing() { | ||
| return os.Setenv((string)(v), value) |
There was a problem hiding this comment.
All of the (string)(v) casts should be able to be written as string(v)
| return fmt.Sprintf("%s=%s", (string)(v), v.Value()) | ||
| } | ||
|
|
||
| // Missing reports as an error listing all Variable among vars that are |
There was a problem hiding this comment.
| // Missing reports as an error listing all Variable among vars that are | |
| // Missing returns an error listing all Variable among vars that are |
| // Map converts a slice of Variable into a map. | ||
| // Its usage is for logging purposes. | ||
| func Map(vars ...Variable) map[string]interface{} { | ||
| result := map[string]interface{}{} |
There was a problem hiding this comment.
Usemake(map[string]any) to instantiate the map (https://gobyexample.com/maps)
any is now equivalent to interface{} and is generally preferable now.
| required = []environment.Variable{ | ||
| port, | ||
| cache.Host, | ||
| namespace, | ||
| refresherImage, | ||
| } | ||
|
|
||
| env = environment.Map(required...) |
There was a problem hiding this comment.
same note on the varaidic
There was a problem hiding this comment.
See #27158 (comment)
| func vars(ctx context.Context) error { | ||
| redisClient := redis.NewClient(&redis.Options{ | ||
| Addr: cache.Host.Value(), | ||
| }) | ||
|
|
||
| cacheClient := (*cache.RedisCache)(redisClient) | ||
|
|
||
| spec.Cache = cacheClient | ||
| spec.Publisher = cacheClient | ||
|
|
||
| k8sClient, err := k8s.NewDefaultClient() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| ns := k8sClient.Namespace(namespace.Value()) | ||
| spec.JobsClient = k8sClient.Jobs(ns) | ||
|
|
||
| if err := redisClient.Ping(ctx).Err(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| logger.Info(ctx, "pinged cache host ok", logging.String("host", cache.Host.Value())) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Same note about the function name from the echo package
There was a problem hiding this comment.
I agree. Thank you.
| if err := redisClient.Ping(ctx).Err(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| logger.Info(ctx, "pinged cache host ok", logging.String("host", cache.Host.Value())) |
There was a problem hiding this comment.
Move the liveness check for the redis client above the kubernetes client work
There was a problem hiding this comment.
Will do. Thank you.
| ) | ||
|
|
||
| var ( | ||
| port environment.Variable = "PORT" |
There was a problem hiding this comment.
May be worth having an extra utils package were you can stash shared environment variables like this and any common functions between the packages for ease of use and improvement
There was a problem hiding this comment.
Good point. I'll have a cmd/common package that provides these shared environment variable names.
| cacheRefresher cache.Refresher | ||
| subscriber cache.Subscriber | ||
|
|
||
| env = logging.Any("env", environment.Map(required...)) |
There was a problem hiding this comment.
Please see #27158 (comment). Though in all cases I should have env = logging.Any("env", environment.Map(required...)). It's private and really only meant for re-usable debugging. When you deploy resources in Kubernetes, unless you clearly show what is missing or not configured, can be cumbersome without a consistent method to report this in the logs.
|
As I've been using this in a Beam context, it has changed a lot and necessitates closing this PR. I will incorporate your helpful feedback in a new PR and rely on any automated assignment of a new reviewer. Thank you again! |
This PR closes #27088 by creating a mock endpoint that simulates a quota aware API. The purpose of this mock endpoint is to collect data as part of the Proposal | Exploring existing Beam features to prevent web service API overuse.
See https://github.com/damondouglas/beam/tree/api-overuse-study-mock-endpoint/.test-infra/pipelines#api-overuse for an overview.
I ran the following prior to submitting this PR:
./gradlew beam-test-infra-pipelines:checkThe go integration test assumes deployed resources. See https://github.com/damondouglas/beam/tree/api-overuse-study-mock-endpoint/.test-infra/pipelines#api-overuse for details.
The following lists what this PR contributes:
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.