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

feat: tracing for gRPC middleware #1086

Merged
merged 3 commits into from
Apr 3, 2023
Merged

feat: tracing for gRPC middleware #1086

merged 3 commits into from
Apr 3, 2023

Conversation

alnr
Copy link
Collaborator

@alnr alnr commented Apr 3, 2023

The diff is much smaller when ignoring whitespace.

@alnr alnr requested a review from zepatrik April 3, 2023 10:10
@alnr alnr self-assigned this Apr 3, 2023
@alnr alnr requested a review from aeneasr as a code owner April 3, 2023 10:10
log.WithError(err).Warn("could not build HTTP request")
span.SetAttributes(attribute.String("oathkeeper.verdict", "denied"))
span.SetStatus(codes.Error, err.Error())
span.End()
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it ended on defer anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, removed

span.SetAttributes(attribute.String("oathkeeper.verdict", "denied"))
span.SetStatus(codes.Error, err.Error())
span.End()
return nil, err
Copy link
Member

Choose a reason for hiding this comment

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

Previously we returned ErrDenied in all these cases. I think that should stay that way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, this was from a previous iteration

@alnr alnr requested a review from zepatrik April 3, 2023 10:35
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #1086 (2e70a24) into master (0f42d7c) will increase coverage by 0.46%.
The diff coverage is 67.74%.

❗ Current head 2e70a24 differs from pull request most recent head b32e7b7. Consider uploading reports for the commit b32e7b7 to get more accurate results

@@            Coverage Diff             @@
##           master    #1086      +/-   ##
==========================================
+ Coverage   77.94%   78.40%   +0.46%     
==========================================
  Files          80       80              
  Lines        3935     3834     -101     
==========================================
- Hits         3067     3006      -61     
+ Misses        589      556      -33     
+ Partials      279      272       -7     
Impacted Files Coverage Δ
middleware/grpc_middleware.go 72.41% <67.21%> (-5.67%) ⬇️
driver/registry_memory.go 90.45% <100.00%> (ø)

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@zepatrik zepatrik merged commit c60e4ac into master Apr 3, 2023
@zepatrik zepatrik deleted the grpc-middleware-tracing branch April 3, 2023 11:25
aeneasr pushed a commit that referenced this pull request Apr 6, 2023
…1084)

* feat: tracing for gRPC middleware (#1086)

* chore(deps): bump github.com/opencontainers/runc from 1.1.4 to 1.1.5

Bumps [github.com/opencontainers/runc](https://github.com/opencontainers/runc) from 1.1.4 to 1.1.5.
- [Release notes](https://github.com/opencontainers/runc/releases)
- [Changelog](https://github.com/opencontainers/runc/blob/v1.1.5/CHANGELOG.md)
- [Commits](opencontainers/runc@v1.1.4...v1.1.5)

---
updated-dependencies:
- dependency-name: github.com/opencontainers/runc
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Arne Luenser <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants