Migrate Jaeger internal packages to OTEL contrib and update import paths #37999
Conversation
github-actions
bot
added
cmd/telemetrygen
github-actions
bot
requested review from
andrzej-stencel,
codeboten,
crobert-1,
Erog38,
frzifus,
MovieStoreGuy,
mx-psi,
yotamloe and
yurishkuro
| } | ||
|
|
||
| // Factory creates new metrics | ||
| type Factory interface { |
There was a problem hiding this comment.
Hey @yurishkuro, you recommended replacing the metrics with the OTEL metrics SDK. With respect to that I have a quick question—should we be relying on the metric tags (like service=my_service) for setting the OTEL service, or do we have a dedicated service input (maybe from config/env) that sets the OTEL resource’s service.name?
Could you please point to where we might be getting this service information.
And could you clarify if I am on the right track.
Thanks!
There was a problem hiding this comment.
In agent metrics the service is from the observed spans, not the collector service.
There was a problem hiding this comment.
Yeah I looked into it and I tried an implementation with otel. Please review the same
There was a problem hiding this comment.
Can you remove all the metrics packages from this PR? Leave only the business logic you're adding.
There was a problem hiding this comment.
Hey, so I reverted to the old metric implementation and kept the rest of the business logic as is.
Does that align with the requirements?
yurishkuro
removed
cmd/telemetrygen
| @@ -1,6 +1,7 @@ | |||
| // Copyright The OpenTelemetry Authors | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
| // | |||
| // Copyright The OpenTelemetry Authors | |||
| // but not the metrics wrapper (because its metric names are specific to agent). | ||
|
|
||
| // ClientConfigManager decides which sampling strategy a given service should be using. | ||
| type ClientConfigManager interface { |
There was a problem hiding this comment.
We said this isn't needed in jaeger receiver
|
|
||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/jaegertracing/jaeger/pkg/testutils" |
There was a problem hiding this comment.
Don't bring more dependencies from jaeger
| ) | ||
|
|
||
| // ConnBuilder Struct to hold configurations | ||
| type ConnBuilder struct { |
There was a problem hiding this comment.
Why is this needed at all? It's for communicating out of the agent to the collector, such function does not exist in OTEL receiver
TODO : Fix metrics -> use OTEL SDK for metrics
Description
Refer to this jaeger issue : jaegertracing/jaeger#6704. Author of the issue: yurishkuro
This PR is a follow-up to the PR - #37956 -> It deviates too much from it's initial description.
This PR moves the necessary Jaeger internal code, into the OTEL collector contrib repository in reference to the mentioned Jaeger Issue. The goal is to move certain dependencies as mentioned in the above issue from jaeger to this codebase :
Code Migration:
Files moved to
receiver/jaegerreceiver/internalandcmd,internal,pkgdirectories contain the files from jaeger codebase. The choice of the folder names and structure was in conjecture to the format used in jaeger codebase.The files were then imported to
receiver/jaegerreceiver/jaeger_agent_test.goandreceiver/jaegerreceiver/trace_receiver.goImport Path Updates:
All import statements have been modified as needed to make it work smoothly.
Dependency Cleanup:
Removing dependencies on Jaeger’s internal packages, thereby resolving build issues related to importing internal modules from an external repository.
Coordination with Jaeger:
This change is part of a broader effort affecting both Jaeger and OTEL contrib. The corresponding Jaeger PR that updates its references to the new OTEL contrib location is tracked separately.
Link to tracking issue
Fixes Jaeger issue #6408
Testing
Documentation