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

Move otelhttp/internal/semconvutil into otelhttp #4580

Open
els0r opened this issue Nov 15, 2023 · 12 comments
Open

Move otelhttp/internal/semconvutil into otelhttp #4580

els0r opened this issue Nov 15, 2023 · 12 comments
Labels
area: instrumentation Related to an instrumentation package enhancement New feature or request instrumentation: otelhttp

Comments

@els0r
Copy link

els0r commented Nov 15, 2023

Problem Statement

The semconvutil package follows semantic conventions for span attributes set during a round-trip of a client-request.

Exposing the utility functions from semconvutil would allow other RoundTrippers to use them, leading to consistent, semconv-aligned telemetry.

In my specific use case, I'm providing a logging transport and would like to use the same attribute keys as the otelhttp transport does.

Proposed Solution

Move semconvutil from go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconvutil to go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/semconvutil

Alternatives

  • Copy-pasting the internals of semconvutil to internal instrumentation library. Which isn't really maintainable
  • Or keep the current implementation, which doesn't align with the semantic conventions
  • Accessing the attributes of the parent span from the otelhttp transport (if that's even possible)

Prior Art

Not aware of any.

Additional Context

We have some tools which run a low frequency of client requests and would like to correlate log messages in our system with the corresponding spans we export.

@els0r els0r added area: instrumentation Related to an instrumentation package enhancement New feature or request instrumentation: otelhttp labels Nov 15, 2023
@els0r els0r changed the title Move go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconvutil into otelhttp Move otelhttp/internal/semconvutil into otelhttp` Nov 15, 2023
@els0r els0r changed the title Move otelhttp/internal/semconvutil into otelhttp` Move otelhttp/internal/semconvutil into otelhttp Nov 15, 2023
@MrAlias
Copy link
Contributor

MrAlias commented Nov 15, 2023

I don't think we want to expose this package without a serious review. It is not designed with stability in mind and is likely going to have major breaking changes as the semantic conventions evolve.

@els0r
Copy link
Author

els0r commented Nov 16, 2023

Thanks for the context on https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/internal/shared/semconvutil

Even if the semantic conventions evolve, it would be nice to stay consistent with the ones that are currently in use by the instrumentation libraries in question.

What would you recommend as a way to go forward? What I can think of right now is to

  • generate httpconv.go from the template and store it inside our own instrumentation librarie's internal folder
  • monitor for changes of that file (as in: newer versions of semconv being referenced)
  • rinse & repeat

Does that make sense or do you see another route to re-use as much code as possible?

@pellared
Copy link
Member

Does that make sense or do you see another route to re-use as much code as possible?

It makes sense. This is what I suggested a few times.

@StarpTech
Copy link

Hi, what's the status? I want to implement my own transport instrumentation but can't use the same semconvutil because the package is internal.

@dmathieu
Copy link
Member

This hasn't moved yet. The stability of otelhttp, and semconvutil is still not there.

@StarpTech
Copy link

StarpTech commented Mar 27, 2024

I got it, but we already rely on it in the instrumentations. What's the difference of consuming them directly from using them indirectly? In both cases, a breaking change could break my system. Therefore e.g. go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp is still under v1. An option could be to export the module through the instrumentation libraries. That would help me to ship customization's without copying the whole code.

@dmathieu
Copy link
Member

There's a difference between providing backwards compatibility with the provided intrumentation (the attributes remain compatible), and providing a backwards compatible API. The latter is a much bigger constraint.

@StarpTech
Copy link

StarpTech commented Mar 27, 2024

There are no API guarantees below v1. Several breaking changes has been made since. #4707 (comment) I don't want to start a general discussion about backwards compatibility. I just see the need that people want to rely on the semconvutil to not reinvent the wheel. I see no reason why we should not export them with the same guarantees as the modules that rely on it. Considering, that both are below semver v1.

@pellared
Copy link
Member

pellared commented Mar 27, 2024

I think we could consider exporting (exposing) helper functions in otelhttp that create spans and metrics for incoming and outgoing requests. This could would allow libraries that work on top of net/http to "natively" instrument the package or make it easier to create instrumentation libraries outside of https://github.com/open-telemetry/opentelemetry-go-contrib.

I think it will not have that much issues like we did when semconvutil was in https://github.com/open-telemetry/opentelemetry-go.

We would need to make sure to properly design the new API to make it usable and forward-compatible.

However, first I would ask why the users cannot simply use otelhttp? Maybe we can design otelhttp (probably as v2) in a way that such helper functions would not be needed. Less is more.

@els0r @StarpTech, can you please share if you could use otelhttp instead? If not, what is the blocker?

@tina-junold
Copy link

I'm having the same problem. For me, with the "tempoary" deprecation of otelecho, it was the point to create an own implementation. But this is nearly impossible unless you have access to semconvutil without reinvent the wheel for attributes and status.

oteltrace.WithAttributes(semconvutil.HTTPServerRequest(service, request)...),
//[...]
span.SetStatus(semconvutil.HTTPServerStatus(status))

In my case, I created a middlware that handle metrics, logs & traces in one place which has some benefits (for me). but the only way was to "copy" the "semconvutil" to my code whitch is not a viable solution.

@StarpTech
Copy link

@pellared main reason is the rapid evolution of semconv attributes in the middleware. We distribute software and rely on the exact attribute naming. We can't just upgrade and don't want to when naming changes or new attributes are added.

@tina-junold
Copy link

Any news on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrumentation Related to an instrumentation package enhancement New feature or request instrumentation: otelhttp
Projects
None yet
Development

No branches or pull requests

6 participants