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

otelgrpc: add WithMeterProvider option #1094

Closed

Conversation

codeboten
Copy link
Contributor

Adding the ability to set a MeterProvider to otelgrpc instrumentation for future use.

@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #1094 (f3ef15c) into main (8cf7954) will decrease coverage by 0.0%.
The diff coverage is 20.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1094     +/-   ##
=======================================
- Coverage   69.4%   69.3%   -0.1%     
=======================================
  Files        135     135             
  Lines       6113    6118      +5     
=======================================
+ Hits        4243    4244      +1     
- Misses      1742    1746      +4     
  Partials     128     128             
Impacted Files Coverage Δ
...ation/google.golang.org/grpc/otelgrpc/grpctrace.go 53.8% <20.0%> (-3.7%) ⬇️

@bogdandrutu
Copy link
Member

@codeboten please rebase

@Aneurysm9 @MrAlias please help us with this PR.

@bogdandrutu
Copy link
Member

@codeboten please rebase

@codeboten
Copy link
Contributor Author

@bogdandrutu done. @open-telemetry/go-approvers would be great to get some feedback on this PR before I need to rebase again, thanks!

@bogdandrutu
Copy link
Member

@codeboten you need another rebase

@codeboten
Copy link
Contributor Author

@bogdandrutu I'm not seeing anything needing rebase in github 🤔 i did rebase yesterday though.

@MrAlias
Copy link
Contributor

MrAlias commented Nov 3, 2021

@codeboten do you have a plan or design doc/issue for the future metric instruments planned to be added here? Is this a PR you plan to submit after this?

I want to be sure we don't couple this instrumentation to the unstable metrics signal without a plan forward with how it will be used.

@codeboten
Copy link
Contributor Author

@codeboten do you have a plan or design doc/issue for the future metric instruments planned to be added here? Is this a PR you plan to submit after this?

I want to be sure we don't couple this instrumentation to the unstable metrics signal without a plan forward with how it will be used.

The metrics I would expect we would want out of this instrumentation would likely be what's defined in the spec here.

This PR is specifically to allow the grpc instrumentation to configure a meter provider in the collector (see open-telemetry/opentelemetry-collector#4030) as we have the ability for today with the tracer provider.

I don't immediately plan on adding metrics to the instrumentation as you mentioned the stability isn't there yet, but i can add an issue and follow up on it at a later time if that works for you.

@codeboten
Copy link
Contributor Author

@MrAlias is there a way to move this PR forward or would you prefer to close it and reinstate when the metrics api is stable? I'm not sure how to move forward at this point.

@codeboten codeboten force-pushed the codeboten/add-meter-provider branch from 0cd881b to 7632a0f Compare April 1, 2022 15:47
@bogdandrutu
Copy link
Member

@MrAlias @codeboten the metrics API is stable, let's move forward with this PR :)

@MrAlias
Copy link
Contributor

MrAlias commented Oct 27, 2022

It looks like #2700 supersedes this, closing. Please reopen if this was in error.

@MrAlias MrAlias closed this Oct 27, 2022
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

6 participants