Skip to content

Tonic Interceptor #901

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

Merged
merged 5 commits into from
Mar 19, 2023
Merged

Tonic Interceptor #901

merged 5 commits into from
Mar 19, 2023

Conversation

blogle
Copy link
Contributor

@blogle blogle commented Oct 24, 2022

Adds optional config for an interceptor to enable modifying each outbound metrics request.

For example - my company has a centralized opentelemetry-collector that gives our SRE team flexibility to determine where they should pipe and store metrics that feed into grafana. This central collector sits behind google IAP. Each internal service needs to supply an oauth token in the grpc headers to publish metrics to this endpoint. This PR exposes the machinery for applications to access the request object and supply such metadata.

@blogle blogle requested a review from a team October 24, 2022 20:25
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 24, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@jtescher jtescher left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started, makes sense to have interceptor support, added some comments.

@blogle blogle force-pushed the tonic-interceptor branch from 12833e1 to 307478f Compare January 18, 2023 19:12
@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (57147b1) 69.9% compared to head (32f6af3) 69.9%.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #901   +/-   ##
=====================================
  Coverage   69.9%   69.9%           
=====================================
  Files        116     116           
  Lines       9027    9027           
=====================================
  Hits        6316    6316           
  Misses      2711    2711           

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

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@blogle blogle requested a review from jtescher January 20, 2023 22:03
@blogle
Copy link
Contributor Author

blogle commented Feb 6, 2023

bump @jtescher

Is anything else you would like to see on this PR? Getting this auth setup working is becoming a higher priority for our team, so would be great to get this integrated.

@TommyCpp
Copy link
Contributor

Sorry didn't get a chance to take a look over the weekend. I will take a look before cut 0.19 release

@blogle
Copy link
Contributor Author

blogle commented Mar 6, 2023

Is this the only item going into the 0.19 release? If so what needs to be done to move this forward? We would really like to start migrating our metrics, but this has been a blocking issue for us.

@cijothomas
Copy link
Member

@blogle Not a comment on the PR content, but a general rule - the CLA check must be cleared before any PRs can be accepted. Could you sign the CLA check? (you may need to consult your employer to see if there are any legal steps to be followed)

Copy link
Contributor

@hdost hdost left a comment

Choose a reason for hiding this comment

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

Looks like you've addressed @jtescher suggestions but yes you'll need to need to sign the CLA.
Thank you for your contribution!

@TommyCpp
Copy link
Contributor

Probably need to address the lint issue also. But I can take a look if needed

@blogle
Copy link
Contributor Author

blogle commented Mar 17, 2023

CLA should now be cleared up. @TommyCpp as for the lints, I believe those pub warnings are pre-existing as I don't think I touched visibility of anything. That said, if this is blocking the PR, I can probably make those all pub(crate) since TonicExporterBuilder is the actual public interface there.

@TommyCpp
Copy link
Contributor

Looks like it needs your approval cc @jtescher

@TommyCpp
Copy link
Contributor

I have pushed a change to fix the lint issue and make the configurations public

@TommyCpp TommyCpp merged commit 675de34 into open-telemetry:main Mar 19, 2023
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.

None yet

5 participants