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

[DO NOT MERGE] Rename db|messaging|gen_ai.system to *.provider.name, rpc.system to rpc.protocol.name #1613

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Nov 26, 2024

Fixes #1581

Warning

THIS IS A VERY-VERY-VERY BREAKING CHANGE (even though it affects experimental attributes only).
Backends use presence of these attributes as an indication that corresponding spans follow certain conventions.

  • *.provider.name is
  • rpc.protocol.name is more precise for RPC where it captures different application-or-higher-level protocol that may work on top of another application protocol (grpc/thrift over http/2)

Despite being problematic, we believe this change is necessary to accommodate future extension of semantic conventions for *.system.* and this is the last chance to make this attribute name right before we declare any of these attributes stable.

Merge requirement checklist


Given potentially high impact of this change and also upcoming holiday season, we'll need to keep it open for a while to collect the feedback

…ider.name, pc.system to pc.protocol.name, feature_flag.system back to feature_flag.provider_name
@lmolkova lmolkova requested review from a team as code owners November 26, 2024 01:40
- rename_attributes:
attribute_map:
feature_flag.provider_name: feature_flag.system
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to extract it to another PR that we can merge right now to reduce churn in feature-flag area

@@ -33,7 +33,6 @@ with the naming guidelines for RPC client spans.

| Attribute | Type | Description | Examples | [Requirement Level](https://opentelemetry.io/docs/specs/semconv/general/attribute-requirement-level/) | Stability |
|---|---|---|---|---|---|
| [`rpc.system`](/docs/attributes-registry/rpc.md) | string | The value `aws-api`. | `aws-api` | `Required` | ![Experimental](https://img.shields.io/badge/-experimental-blue) |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe it's used correctly here

Copy link
Member

@Flarna Flarna Dec 3, 2024

Choose a reason for hiding this comment

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

I think it is, AWS SDK is modeled as remote call. There is #1622 to add aws_api to the well known enums.

Anyhow, removing rpc.system but keep rpc.method and rpc.service doesn't seem right.

Copy link
Contributor Author

@lmolkova lmolkova Dec 3, 2024

Choose a reason for hiding this comment

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

it's not a remote call though - it's a logical call that uses HTTP underneath. OTel RPC semconv represent actual remote calls using remote calling protocols.

Copy link
Member

Choose a reason for hiding this comment

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

Well, that's more or less the same for GRPC which uses HTTP/2 as transport.
Anyhow, likely a topic for a different issue.

But still I think you should either keep or remove all rpc attributes here.

@lmolkova lmolkova changed the title Rename db|messaging|gen_ai.system to *.provider.name, rpc.system to rpc.protocol.name, feature_flag.system back to feature_flag.provider_name [DO NOT MERGE] Rename db|messaging|gen_ai.system to *.provider.name, rpc.system to rpc.protocol.name Nov 26, 2024
@codefromthecrypt
Copy link

codefromthecrypt commented Dec 10, 2024

I have some opinions on this, which I don't expect to be accepted, but would like on the record even if dismissed.

As genai is typically accessed from web service apis which are commodity and easier than the typical case of database protocols, I would prefer changes to be discussed independently.

For the case of genai, I do strongly prefer whatever word we use for api (e.g. openai which is often emulated) to be decoupled from the provider of that. However, I would like this to advise to use the same constant even if in some cases the client might get it wrong. For example, don't say something like "azure inference client" as the system or provider name, for when it is being used to access azure openai service with the openai api. Especially a proxy client (litellm also has a proxy client library), should attempt to do more than say its name.

The field will be used for both sides, and it seems a step back to end up with client SDK names in either of these fields, vs a web service api, possibly private provider of that. The edge case of local llms, e.g. running llama.cpp embedded, we can discuss, too, but most of the time this is about web services.

more below if you like


I think there is ongoing tension treating genai like database, when in practice they are typically accessed via web services apis and so more like a normal cloud service than a database. This becomes even more the case as more and more are instrumented server side, or as proxies. For example, while there are indeed some L7 proxies for database protocols, it is a lot more common and easier to do on genai as most if not all well known services are web services. So, there are backends who instrument the http server side like a normal private cloud product, as well as proxies who instrument both ways today.

So, I would like to decouple the topic of database from genai, especially this sense of provider being implied as a client identifier. I'm not sure "provider" or "system" for that matter intuitively mean "client perceived name" especially when the attribute isn't namespaced as client and also used for server data (in practice even if only in genai metrics are the only server thing so far).

I cofounded a project back in 2009 called jclouds which has very much overlap on the problem of api vs provider of that. When the community got together, probably the single best feature we did was formalizing "api" independent of "provider" of that. So, while I do like the idea of using provider, I would like it to be more strong, so it at least suggests to use an authoritative name for the actual service as opposed to a client SDK name, especially in SDKs that target multiple services.

https://jclouds.apache.org/start/concepts/#:~:text=Providers%20implement%20APIs%2C%20and%20describe,those%20offered%20by%20the%20API.

@lmolkova
Copy link
Contributor Author

@codefromthecrypt can you suggest what'd you like to be changed/added ?

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

Should db.system be db.system.name?
3 participants