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

Expose instrumentation scope name #4448

Merged
merged 19 commits into from
Nov 8, 2023
Merged

Conversation

utezduyar
Copy link
Contributor

@utezduyar utezduyar commented Oct 20, 2023

It is good to know the scope to apply view filters. For example if we would like to drop all or some of the meters from this library, below code is how the filtering is applied. The scope name is used explicitly and a change in the scope name by the runtime instrumentation library will break applications.

metricsdk.Instrument{Name: "*",
Scope: instrumentation.Scope{
	Name:    "go.opentelemetry.io/contrib/instrumentation/runtime",
}},
metricsdk.Stream{Aggregation: metricsdk.AggregationDrop{}},

Instead of this, get the scope name from the library itself

metricsdk.Instrument{Name: "*",
Scope: instrumentation.Scope{
	Name:    runtime.Name(),
}},
metricsdk.Stream{Aggregation: metricsdk.AggregationDrop{}},

@utezduyar utezduyar requested a review from a team October 20, 2023 05:55
@pellared
Copy link
Member

pellared commented Oct 20, 2023

Could you please make the the PR description more detailed?

You could e.g. give examples with code what is possible with the new method and what is the alternative when it is missing (changing to draft before it is done).

@pellared pellared marked this pull request as draft October 20, 2023 06:16
@utezduyar
Copy link
Contributor Author

@pellared How does it look now?

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

How does it look now?

Now, I find it clear 👍

I will first change the the title and ask others if they like the proposal.

If the proposal would be accepted then:

  1. We would have to do this for all instrumentation libraries.
  2. Changelog would have to be updated.

instrumentation/runtime/runtime.go Outdated Show resolved Hide resolved
@pellared pellared changed the title intrumentation-runtime: expose scope [DoNotMerge] [Proposal] Expose instrumentation scope name Oct 20, 2023
@pellared
Copy link
Member

pellared commented Oct 20, 2023

@open-telemetry/go-approvers PTAL

Give the PR a 👍 if you like the proposal.
Give 👎 if you are against.

Additionally more detailed (text) feedback is welcome.

@pellared pellared marked this pull request as ready for review October 20, 2023 09:16
@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

Merging #4448 (b8d4418) into main (2b69029) will not change coverage.
The diff coverage is 80.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4448   +/-   ##
=====================================
  Coverage   80.8%   80.8%           
=====================================
  Files        150     150           
  Lines      10371   10371           
=====================================
  Hits        8387    8387           
  Misses      1840    1840           
  Partials     144     144           
Files Coverage Δ
.../github.com/aws/aws-lambda-go/otellambda/lambda.go 89.0% <100.0%> (ø)
...tation/github.com/aws/aws-sdk-go-v2/otelaws/aws.go 94.6% <100.0%> (ø)
...hub.com/emicklei/go-restful/otelrestful/restful.go 100.0% <100.0%> (ø)
...trumentation/github.com/gorilla/mux/otelmux/mux.go 89.2% <100.0%> (ø)
...entation/github.com/labstack/echo/otelecho/echo.go 100.0% <100.0%> (ø)
...entation/google.golang.org/grpc/otelgrpc/config.go 69.6% <100.0%> (ø)
...ion/google.golang.org/grpc/otelgrpc/interceptor.go 87.8% <100.0%> (ø)
...ntation/gopkg.in/macaron.v1/otelmacaron/macaron.go 91.4% <100.0%> (ø)
...on/net/http/httptrace/otelhttptrace/clienttrace.go 79.0% <100.0%> (ø)
instrumentation/net/http/otelhttp/common.go 100.0% <100.0%> (ø)
... and 5 more

@utezduyar
Copy link
Contributor Author

  1. We would have to do this for all instrumentation libraries.

What other libraries are you aware of? I can get to them too.

@pellared
Copy link
Member

  1. We would have to do this for all instrumentation libraries.

What other libraries are you aware of? I can get to them too.

All under https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/instrumentation

@utezduyar
Copy link
Contributor Author

I think I found all that are using MeterProvider.

@pellared
Copy link
Member

pellared commented Nov 2, 2023

I think I found all that are using MeterProvider.

The same has to be done for all TraceProvider.Tracer usages (like you did in otelhttp).

@utezduyar
Copy link
Contributor Author

I think I found all that are using MeterProvider.

The same has to be done for all TraceProvider.Tracer usages (like you did in otelhttp).

otelhttp has a meterprovider that is why I did it. Do we have a use case for exposing the scope names for tracers? Of course I don't mind making the change. I am just curious.

@pellared
Copy link
Member

pellared commented Nov 2, 2023

Do we have a use case for exposing the scope names for tracers?

API consistency.

For now, leave it as it is. I added your proposal as a topic for today's SIG meeting

@pellared
Copy link
Member

pellared commented Nov 2, 2023

@utezduyar During SIG meeting we agreed that

  1. we accepted your proposal
  2. we want to have ScopeName for all instrumentation libraries and reuse it when creating meters and tracers
  3. CHANGELOG.md entry must be added

I am converting the PR to draft. Please make it "Ready for review" when you update the PR.

@pellared pellared marked this pull request as draft November 2, 2023 21:02
@pellared pellared changed the title [DoNotMerge] [Proposal] Expose instrumentation scope name Expose instrumentation scope name Nov 2, 2023
@utezduyar utezduyar marked this pull request as ready for review November 3, 2023 08:08
@pellared
Copy link
Member

pellared commented Nov 7, 2023

What about ScopeName for otelgrpc?

@utezduyar
Copy link
Contributor Author

What about ScopeName for otelgrpc?

It is back again.

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I noticed that we are also missing ScopeName in otelmacaron and otelhttptrace.

@utezduyar
Copy link
Contributor Author

otelhttptrace

Can you point me the file name for this? I still couldn't find it.

@pellared
Copy link
Member

pellared commented Nov 8, 2023

otelhttptrace

Can you point me the file name for this? I still couldn't find it.

instrumentation/net/http/httptrace/otelhttptrace/clienttrace.go

instrumentation/README.md Outdated Show resolved Hide resolved
Co-authored-by: David Ashpole <dashpole@google.com>
@pellared
Copy link
Member

pellared commented Nov 8, 2023

@utezduyar Thank you for your contribution 🎉

@pellared pellared merged commit a3b16ae into open-telemetry:main Nov 8, 2023
21 of 22 checks passed
@utezduyar
Copy link
Contributor Author

@utezduyar Thank you for your contribution 🎉

Thanks for your assistance.

@pellared pellared added this to the untracked milestone Nov 8, 2024
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.

4 participants