Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module/apmgrpc: add stream interceptors #918

Merged
merged 2 commits into from
Mar 25, 2021

Conversation

axw
Copy link
Member

@axw axw commented Mar 24, 2021

Add server and client stream interceptors. Very similar to unary interceptors -- the transaction and span represents the entire gRPC stream.

Client spans are ended when the stream is closed, which is observed when stream methods return with an error, or for unary servers, RecvMsg returns (not necessarily with an error). This is very similar to the client wrapper in apmhttp, where the span is ended only after the response body is consumed.

Closes #901

@apmmachine
Copy link
Contributor

apmmachine commented Mar 24, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #918 updated

  • Start Time: 2021-03-25T01:22:12.231+0000

  • Duration: 25 min 18 sec

  • Commit: ad1d15f

Test stats 🧪

Test Results
Failed 0
Passed 9979
Skipped 266
Total 10245

Trends 🧪

Image of Build Times

Image of Tests

@axw axw force-pushed the apmgrpc-stream-interceptor branch from 62dd126 to 6be2edb Compare March 24, 2021 05:57
@codecov-io
Copy link

Codecov Report

Merging #918 (6be2edb) into master (df272e4) will decrease coverage by 0.18%.
The diff coverage is 71.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #918      +/-   ##
==========================================
- Coverage   83.79%   83.61%   -0.19%     
==========================================
  Files         140      141       +1     
  Lines        7895     8036     +141     
==========================================
+ Hits         6616     6719     +103     
- Misses        877      911      +34     
- Partials      402      406       +4     
Impacted Files Coverage Δ
module/apmgrpc/server.go 80.76% <54.28%> (-13.44%) ⬇️
...ule/apmgrpc/internal/testservice/testservice.pb.go 60.00% <60.00%> (ø)
module/apmgrpc/client.go 94.06% <89.83%> (-4.24%) ⬇️
module/apmgrpc/ignorer.go 75.00% <100.00%> (+8.33%) ⬆️
tracer.go 88.58% <0.00%> (+0.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df272e4...6be2edb. Read the comment docs.

@axw axw marked this pull request as ready for review March 24, 2021 06:40
@axw axw requested a review from a team March 24, 2021 06:40
Copy link
Contributor

@stuartnelson3 stuartnelson3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment on a TODO but other than that looks good

defer tx.End()

// TODO(axw) define context schema for RPC,
// including at least the peer address.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is context span.Context here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I'll reword to clarify

@axw axw merged commit f936e4b into elastic:master Mar 25, 2021
@axw axw deleted the apmgrpc-stream-interceptor branch March 25, 2021 02:41
stuartnelson3 pushed a commit that referenced this pull request Jul 30, 2021
* module/apmgrpc: add stream interceptors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gRPC Stream Interceptors
4 participants