-
Notifications
You must be signed in to change notification settings - Fork 290
Introduce service.peer.name and service.peer.namespace; deprecate peer.service #3097
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
base: main
Are you sure you want to change the base?
Conversation
14b1a2b to
6b843a7
Compare
thompson-tomo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR should be Blocked due to old attributes not actually being deprecated but rather only a single usage of the attribute being deprecated. Also there is 2 change logs when there should only be 1.
If we think about the use of the attributes for me it makes more sense to put these new attributes under the server namespace given that they are being set based on the server address/port.
It would also enable the server namespace to be the sole namespace containing attributes describing the server side of a connection and the service namespace to be describing the service being used.
| - id: service.peer.name | ||
| type: string | ||
| stability: development | ||
| brief: > | ||
| Logical name of the service on the other side of the connection. | ||
| SHOULD be equal to the actual [`service.name`](/docs/resource/README.md#service) resource attribute of the remote service if any. | ||
| examples: ["shoppingcart"] | ||
| - id: service.peer.namespace | ||
| type: string | ||
| stability: development | ||
| brief: > | ||
| A namespace for `service.peer.name`. | ||
| examples: ["Shop"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - id: service.peer.name | |
| type: string | |
| stability: development | |
| brief: > | |
| Logical name of the service on the other side of the connection. | |
| SHOULD be equal to the actual [`service.name`](/docs/resource/README.md#service) resource attribute of the remote service if any. | |
| examples: ["shoppingcart"] | |
| - id: service.peer.namespace | |
| type: string | |
| stability: development | |
| brief: > | |
| A namespace for `service.peer.name`. | |
| examples: ["Shop"] | |
| - id: server.service.name | |
| type: string | |
| stability: development | |
| brief: > | |
| Logical name of the service running on the server side of the connection. | |
| SHOULD be equal to the actual [`service.name`](/docs/resource/README.md#service) resource attribute of the remote service if any. | |
| examples: ["shoppingcart"] | |
| - id: server.service.namespace | |
| type: string | |
| stability: development | |
| brief: > | |
| A namespace for `server.service.name`. | |
| examples: ["Shop"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire rationale behind this PR is that we want the service related names to be under the same "authority" / maintainership.
moving into server, again, splits the ownership in ways we do not want.
The goal is when we change service.* attributes, we can more easily see the impact on service.peer attributes and maintain them cohesively.
| deprecated: | ||
| reason: renamed | ||
| renamed_to: service.peer.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be specified on the attribute definition/registry and not the usage of it.
| deprecated: | |
| reason: renamed | |
| renamed_to: service.peer.name |
I am trying to figure out how to make this work the right way. Pointers appreciated :-)
IMO, it is really not a given that these are set on server address/port. It is difficult to generalize on something like this, and it is going to be very implementation-dependent, but more than one implementation of peer.service I have seen happened at a pretty high level, e.g., embedded in client libraries automatically generated from OpenAPI specs. |
Move the deprecated block from common.yaml into the registry.yaml file and then regenerate the docs. The generation process will propagate the deprecation info onto the attributes on the signals.
This is functionality which is defined via declarative config.
That would also be fine and still fit having it under server as server would be describing the remote side of the connection. |
|
hi @mmanciop! thanks for taking this on. we discussed this in yesterday's Service and Deployment Semconv SIG meeting (https://zoom.us/rec/share/W7L9lhkmwN6GGzh1ykMxcF-dr2B0frjOAaO52BSe-8VP-Zag6n15gQObv_sFtqM.OCG1dIwmHpD2D2rP starting around 34:00 minute marker). I believe we're onboard with renaming We'd like a bit more understanding of whether For We still need to decide whether we want to recommend Can you investigate the existing usages of Once we decide on the migration and stabilization plan, we want to communicate it via a short blog post, to hopefully avoid the surprises that folks had with the |
Very nice!
The identity of a service is the combination of its
The canonical example in my head is the
Sure, I had already done the research before coming up with the proposal, and I just spent some more time with GitHuib search to make sure I did not miss anything. To my knowledge, the list is as follow, going through SDKs and contribs in alphabetical order:
Others:
Sounds good to me. I also think that adding one more fallback case for |
6b843a7 to
985629b
Compare
|
This PR contains changes to area(s) that do not have an active SIG/project and will be auto-closed:
Such changes may be rejected or put on hold until a new SIG/project is established. Please refer to the Semantic Convention Areas |
| stability: development | ||
| brief: > | ||
| Logical name of the service on the other side of the connection. | ||
| SHOULD be equal to the actual [`service.name`](/docs/resource/README.md#service) resource attribute of the remote service if any. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What has me concerned here is we don't have a specified way for instrumentation (i.e the OTel API) to access the resource attribute and fill out a service.name here, e.g. if using baggage.
@mmanciop I do think we need an overall "Improve the state of service-peer in OTEL Specification" tracking bug, with known examples/demos you've collected.
However, there's enough usage in OTEL now, that I think we this is ok for now. I just believe this requirement is "too hard" to accomplish in practice in OTEL.
| groups: | ||
| - id: service.peer | ||
| type: attribute_group | ||
| brief: "Operations that access some remote service." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: "Operations that access some remote service MAY fill out these attributes"
Fixes #2945
Changes
Please provide a brief description of the changes here.
Important
Pull requests acceptance are subject to the triage process as described in Issue and PR Triage Management.
PRs that do not follow the guidance above, may be automatically rejected and closed.
Merge requirement checklist
[chore]