[service] Move TracerProvider initialization to service/telemetry package #8171
Conversation
mx-psi
added
area:service
collector-telemetry
mx-psi
force-pushed
the
mx-psi/refactor-service-telemetry
branch
from
7ea601a
to
28300f1
Compare
| // needed for supporting the zpages extension | ||
| sdktrace.WithSampler(alwaysRecord()), |
There was a problem hiding this comment.
This was not present on service/telemetry.go, but was present on service/telemetry/telemetry.go initialization logic, so I added it here.
There was a problem hiding this comment.
This should probably be available as an option in the function, since zpages will always want this on, but users will want the ability to configure their own samplers
There was a problem hiding this comment.
This should probably be available as an option in the function, since zpages will always want this on, but users will want the ability to configure their own samplers
By "an option in the function" you mean an option in newTracerProvider? Would this option be exposed to end users?
Codecov ReportAttention:
... and 1 file with indirect coverage changes 📢 Thoughts on this report? Let us know! |
| // needed for supporting the zpages extension | ||
| sdktrace.WithSampler(alwaysRecord()), |
There was a problem hiding this comment.
This should probably be available as an option in the function, since zpages will always want this on, but users will want the ability to configure their own samplers
| // supported protocols for exporters | ||
| const ( | ||
| // ProtocolProtobufHTTP is the OTLP/HTTP with Protobuf encoding protocol | ||
| ProtocolProtobufHTTP = "http/protobuf" | ||
| // ProtocolProtobufGRPC is the OTLP/gRPC protocol | ||
| ProtocolProtobufGRPC = "grpc/protobuf" | ||
| ) | ||
|
|
There was a problem hiding this comment.
This is now exposed by this package. I think it makes sense to expose it given it is part of the public API
| // NormalizeEndpoint takes an HTTP(s) endpoint and adds the protocol prefix if missing. | ||
| func NormalizeEndpoint(endpoint string) string { | ||
| if !strings.HasPrefix(endpoint, "https://") && !strings.HasPrefix(endpoint, "http://") { | ||
| return fmt.Sprintf("http://%s", endpoint) | ||
| } | ||
| return endpoint | ||
| } |
There was a problem hiding this comment.
This is now exposed by this package and imported by service/internal/proctelemetry. I am not sure if this makes sense and want some feedback on this. Should we move this to a package in service/internal? Should this be a public method on the Otlp configuration struct instead?
cc @codeboten
|
For those following at home, this is blocked by open-telemetry/opentelemetry-go-contrib/pull/4228 after offline conversation with Alex. This will solve the comments above |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Waiting on #8620 |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description:
Moves TracerProvider initialization to
service/telemetrypackage. To do this while avoiding duplicated code and import cycles I also had to:service/internal/resourcepackage to handle resource creation.service/telemetryfromservice/internal/proctelemetryandservice.A second PR will move the meter provider initialization to
service/telemetryfor consistency.Link to tracking Issue: Updates #8170