[RRIO] Define and implement mock quota aware API #28893

damondouglas · 2023-10-09T08:33:30Z

This PR closes #28708. It defines and implements a mock quota aware API. See PR branch's .test-infra/mock-apis/README for more details.

Notes for the reviewer:

The TL;DR of this PR is that it allows us to write Beam tests using an API we can control i.e. the size of the quota, the contract, etc. It hosts both HTTP and gRPC with one codebase.
I recommend you start the review at .test-infra/mock-apis/src/main/go/cmd/service/refresher/main.go and .test-infra/mock-apis/src/main/go/cmd/service/refresher/main.go
I attempted integration tests using testcontainers but given the various dependencies the test code was unreadable and had several race conditions. [Task]: [RRIO] [Testing] Mock API integration tests #28859 addresses the need for integration tests and Infrastructure-as-Code setup to make this cleaner.
The src/main/go path is to be compatible with other proto generated code. This PR includes src/main/java proto generated code. Python needs some further research as it didn't seem as straightforward. Python tests could call the HTTP endpoint directly.
To make the ./gradlew :beam-test-infra-mock-apis:check happy, I had to add some suppress configurations that weren't relevant for this project. If this project were to use the gradle plugin for proto generation, it wouldn't play as nicely with Go, which is arguably more important.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: 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, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

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 or the workflows README to see a list of phrases to trigger workflows.

codecov · 2023-10-09T08:53:24Z

Codecov Report

Merging #28893 (4cb198a) into master (5515f18) will decrease coverage by 33.84%.
Report is 181 commits behind head on master.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #28893       +/-   ##
===========================================
- Coverage   72.22%   38.38%   -33.84%     
===========================================
  Files         684      686        +2     
  Lines      101241   101608      +367     
===========================================
- Hits        73117    39007    -34110     
- Misses      26548    61025    +34477     
  Partials     1576     1576

Flag	Coverage Δ
python	`30.01% <ø> (-52.76%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 314 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

github-actions · 2023-10-09T16:06:33Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @bvolpato for label java.
R: @Abacn for label build.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

Abacn · 2023-10-11T19:42:29Z

thanks for the change, could you please find a go reviewer?

damondouglas · 2023-10-11T19:55:27Z

thanks for the change, could you please find a go reviewer?

I did already thank you.

lostluck · 2023-10-11T20:13:19Z

That would be me :) It's a lot of code to go through.

lostluck · 2023-10-11T20:14:25Z

Damon, in particular, by commenting R: <user> is better than doing github reviewer assignment WRT our automation, and avoids the auto assignment.

damondouglas · 2023-10-11T21:25:51Z

That would be me :) It's a lot of code to go through.

Would you like me to break it up? In the future I'll prevent this.

lostluck · 2023-10-11T21:36:07Z

I'm already halfway through it. Please make no changes at this time, to avoid comment loss.

…

On Wed, Oct 11, 2023, 2:26 PM Damon ***@***.***> wrote: That would be me :) It's a lot of code to go through. Would you like me to break it up? In the future I'll prevent this. — Reply to this email directly, view it on GitHub <#28893 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADKDOFLX3RNBTIMUE5OQLI3X64FGVANCNFSM6AAAAAA5YNTQ6Q> . You are receiving this because your review was requested.Message ID: ***@***.***>

lostluck

I looked at all the Go code, but not the Java code.

I'm very happy with this! I have several mostly minor style nits, but other than that, this is clean and well factored.

.test-infra/mock-apis/src/main/go/cmd/service/echo/main.go

.test-infra/mock-apis/src/main/go/cmd/service/refresher/main.go

.test-infra/mock-apis/src/main/go/internal/environment/variable_test.go

.test-infra/mock-apis/src/main/go/internal/logging/logging.go

.test-infra/mock-apis/src/main/go/internal/service/echo/echo.go

github-actions · 2023-10-21T12:13:54Z

Reminder, please take a look at this pr: @bvolpato @Abacn

lostluck · 2023-10-21T17:25:16Z

Re-adding the Java label. There's ~2k lines of Java here. It gets the Java label.

lostluck

I like what you've done with slog!

This is just a note, do not undo your work.

I will note that you went farther than necessary for plumbing the logger everywhere. It would have also been acceptable and sufficient to override the default logger/handler of slog in your package mains, and then just using the package level logging calls instead of plumbing it through everywhere.

I started looking at the java, and eventually realized it must be generated since it fully qualifies all the types. Including java.lang.String. I didn't understand that from the PR description initially. Sorry about that.

I'm gonna remove that java label.

lostluck · 2023-10-21T17:30:12Z

.test-infra/mock-apis/src/main/go/internal/cache/cache.go

+		},
+	}
+
+	ref.opts.Logger.LogAttrs(ctx, slog.LevelInfo, "starting refresher service", attrs...)


Instead of using attrs... on every call, use the logger.With method early on to get a logger with those outputs built.

https://pkg.go.dev/log/slog#Logger.With

Noting this and thank you. Per your original comment, I won't change anything and just keep this comment open for the future Thank you again.

I mean, I meant not to change the fact that you plumb the logger through everywhere, instead of using the package level slog methods.

You should totally avoid repeatedly unrolling variadic parameters at every call site, where appropriate.

lostluck · 2023-10-21T17:39:32Z

.test-infra/mock-apis/src/main/go/cmd/service/echo/main.go

+		LoggingAttrs: logAttrs,
+		Logger:       logger,


If the intent here is to always print out these attrs with every request, just build them into the logger instead of forcing per-callsite behavior on yourself.

https://pkg.go.dev/log/slog#Logger.With

If the attributes are only going to be logged sometimes though, document the field with the conditions they're logged, so it's clear what that field is for.

Ah this is a good tip thank you. I won't change anything with slog related as you requested not up undo my work. I'll keep this comment open as a future reference.

lostluck · 2023-10-21T17:39:49Z

.test-infra/mock-apis/src/main/go/internal/cache/cache.go

+)
+
+var (
+


rm blank line

lostluck · 2023-10-21T17:48:10Z

.test-infra/mock-apis/src/main/go/internal/metric/doc.go

+// Package metric supports monitoring.
+package metric


Style note: If the documentation is small, and the package is small (file wise), it's better to just put it on top of one ot the non test files. A separate doc file works best when the package documentation is long, or doesn't naturally fit into one of the existing files.

Usually though in that case there'd be a file named the same as the package. If it were me, I'd rename interface.go to metric.go since it's an evident entry point to the package.

I was anticipating it being longer in the beginning but then realized it didn't need to be. I won't change anything at this point though unless you or others feel strongly about it.

lostluck · 2023-10-21T17:51:11Z

.test-infra/mock-apis/src/main/go/internal/service/echo/echo.go

+		return
+	}
+	if err := json.NewEncoder(w).Encode(resp); err != nil {
+		srv.opts.Logger.LogAttrs(context.Background(), slog.LevelError, err.Error(), srv.opts.LoggingAttrs...)


Instead of using context.Background() for these logging calls, use the one attached to the request:

https://pkg.go.dev/net/http#Request.Context

Ah this was a silly oversight on my part. I forgot there is a request.Context(). I will change this in a future PR.

lostluck · 2023-10-21T17:52:10Z

.test-infra/mock-apis/src/main/go/internal/service/echo/echo.go

+// Register a grpc.Server with the echov1.EchoService. Returns a http.Handler or error.
+func Register(s *grpc.Server, opts *Options) (http.Handler, error) {
+	if opts.Logger == nil {
+		opts.Logger = logging.New(&logging.Options{


Same comment here WRT the logger and the always used LoggingAttrs. Use .With() to build them into the logger.

You mentioned not to undo work in the context of slog. Does your comment apply in this context as well? FYI, I will be doing the infrastructure as code PR and may uncover a couple of things in the code that might need fixing so perhaps I can refactor the slog related stuff there.

.test-infra/mock-apis/src/main/java/org/apache/beam/testinfra/mockapis/echo/v1/Echo.java

damondouglas and others added 10 commits October 5, 2023 01:03

Implement gRPC service with transcoder

465bcbd

WIP: defer integration tests for later

1772ca9

Make service executables

1bd0c1d

remove grpc gateway

1586095

Clean up and add messy integration test

547aa33

Delete flakey integration tests that depend on Docker

fafb9e4

Generate java proto

12b39c2

Generate python proto

ec0a8c9

Python is too much trouble for now

f5ebb14

Finalize README

3c36b23

damondouglas requested a review from lostluck October 9, 2023 08:33

github-actions bot added java build infra labels Oct 9, 2023

damondouglas added 2 commits October 9, 2023 05:28

Fix rat and whitespace

e9800e7

Fix whitespace

77dcf09

damondouglas marked this pull request as ready for review October 9, 2023 15:39

github-actions bot added the Next Action: Reviewers label Oct 9, 2023

damondouglas added go and removed java labels Oct 11, 2023

github-actions bot added java and removed go labels Oct 11, 2023

lostluck requested changes Oct 11, 2023

View reviewed changes

damondouglas added 2 commits October 13, 2023 00:58

Clean up per PR comments

f3b53f1

Fix gcp logging not capturing severity

4cb198a

damondouglas requested a review from lostluck October 13, 2023 21:21

github-actions bot added the slow-review label Oct 21, 2023

damondouglas added go and removed java labels Oct 21, 2023

lostluck added the java label Oct 21, 2023

github-actions bot removed the slow-review label Oct 21, 2023

lostluck approved these changes Oct 21, 2023

View reviewed changes

lostluck removed the java label Oct 21, 2023

damondouglas merged commit a07d90e into apache:master Oct 23, 2023
29 of 30 checks passed

damondouglas deleted the rrio/testing/api branch October 23, 2023 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RRIO] Define and implement mock quota aware API #28893

[RRIO] Define and implement mock quota aware API #28893

damondouglas commented Oct 9, 2023 •

edited

Loading

codecov bot commented Oct 9, 2023 •

edited

Loading

github-actions bot commented Oct 9, 2023

Abacn commented Oct 11, 2023

damondouglas commented Oct 11, 2023

lostluck commented Oct 11, 2023

lostluck commented Oct 11, 2023 •

edited

Loading

damondouglas commented Oct 11, 2023

lostluck commented Oct 11, 2023 via email

lostluck left a comment

github-actions bot commented Oct 21, 2023

lostluck commented Oct 21, 2023

lostluck left a comment

lostluck Oct 21, 2023

damondouglas Oct 21, 2023

lostluck Oct 21, 2023

lostluck Oct 21, 2023

damondouglas Oct 21, 2023

lostluck Oct 21, 2023

lostluck Oct 21, 2023

damondouglas Oct 21, 2023

lostluck Oct 21, 2023

damondouglas Oct 21, 2023 •

edited

Loading

lostluck Oct 21, 2023

damondouglas Oct 21, 2023

		)

		var (

[RRIO] Define and implement mock quota aware API #28893

[RRIO] Define and implement mock quota aware API #28893

Conversation

damondouglas commented Oct 9, 2023 • edited Loading

GitHub Actions Tests Status (on master branch)

codecov bot commented Oct 9, 2023 • edited Loading

Codecov Report

github-actions bot commented Oct 9, 2023

Abacn commented Oct 11, 2023

damondouglas commented Oct 11, 2023

lostluck commented Oct 11, 2023

lostluck commented Oct 11, 2023 • edited Loading

damondouglas commented Oct 11, 2023

lostluck commented Oct 11, 2023 via email

lostluck left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 21, 2023

lostluck commented Oct 21, 2023

lostluck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damondouglas Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damondouglas commented Oct 9, 2023 •

edited

Loading

codecov bot commented Oct 9, 2023 •

edited

Loading

lostluck commented Oct 11, 2023 •

edited

Loading

damondouglas Oct 21, 2023 •

edited

Loading