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

[configrpc] Remove SanitizedEndpoint? #9482

Closed
mx-psi opened this issue Feb 6, 2024 · 6 comments · Fixed by #9836
Closed

[configrpc] Remove SanitizedEndpoint? #9482

mx-psi opened this issue Feb 6, 2024 · 6 comments · Fixed by #9836
Labels
area:config release:required-for-ga Must be resolved before GA release

Comments

@mx-psi
Copy link
Member

mx-psi commented Feb 6, 2024

As far as I can tell, this function is not used by any code, either in core or contrib. Should we remove it?

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Feb 21, 2024

I think we need to reconsider removing this feature. #9616 showed the otlpexporter relies on it.

We could reproduce the functionality directly in the otlpexporter, but leaving it in configgrpc seems ok as well.

dmitryax pushed a commit that referenced this issue Feb 27, 2024
**Description:**
Removes deprecated functions/structs

**Link to tracking Issue:** 
Related to
#9428
Related to
#9482
Closes
#9481
@mx-psi
Copy link
Member Author

mx-psi commented Mar 1, 2024

I think we need to reconsider removing this feature. #9616 showed the otlpexporter relies on it.

We could reproduce the functionality directly in the otlpexporter, but leaving it in configgrpc seems ok as well.

@TylerHelmuth Do we have more than one usage of this function? If not, I would vote to move this to the OTLP exporter

@TylerHelmuth
Copy link
Member

@mx-psi it has been used in configgrpc's ToClientConn function for a long time. Looking at usage of ToClientConn, it is used in the otlpexporter, coralogixexporter, opencensusexporter, otelarrowexporter, skywalkingexporter, jaegerremotesamplingextension, and testbed.

@atoulme
Copy link
Contributor

atoulme commented Mar 1, 2024

The OTLP exporter has another way to deal with sanitized endpoint by stripping the scheme of the URL at unmarshaling time.

OK, so for this to happen:

  1. We need embedded struct unmarshaling support [confmap] confmap honors Unmarshal methods on config embedded structs. #9635
  2. Then we can implement an unmarshaler for configgrpc. We need one anyway for [configgrpc] Clean up the logic associated with https endpoints #9537

@TylerHelmuth
Copy link
Member

For the usage within ToClientConn we can make the otlpexporter implement its own version of SanatizeEndpoints since it is the only direct external use and then in configgrpc we can unexport SanitizedEndpoint to leave ToClientConn features intact. This reduces the size of configgrpc's public API with no end-user impact.

@mx-psi
Copy link
Member Author

mx-psi commented Mar 1, 2024

@TylerHelmuth As you said, I don't think uses on configrpc itself count; we can just unexport the function, my concern is about having it as part of the public API.

mx-psi pushed a commit that referenced this issue Mar 19, 2024
**Description:** <Describe what has changed.>
Deprecates `configgrpc.SanitizedEndpoint()`.

**Link to tracking Issue:** <Issue number if applicable>
Works towards:
#9482
mx-psi pushed a commit that referenced this issue Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config release:required-for-ga Must be resolved before GA release
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants