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

added JSON RPC specific attributes #1643

Merged
merged 18 commits into from
May 7, 2021

Conversation

Rast1234
Copy link
Contributor

This is second attempt: PR #1606 was automatically closed by bot.

Tracing attributes for JSON RPC protocol.

Changes

Added attributes:

rpc.jsonrpc.version
rpc.jsonrpc.method
rpc.jsonrpc.id
rpc.jsonrpc.error_code
rpc.jsonrpc.error_message

Not sure if null values are OK for attributes, maybe need to change rpc.jsonrpc.id a bit. Also not sure if error_message is OK because it is arbitrary (but short!) text, can impact cardinality.

Related issue: #1601

@Rast1234 Rast1234 requested review from a team April 24, 2021 12:32
semantic_conventions/trace/rpc.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/rpc.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/rpc.yaml Show resolved Hide resolved
semantic_conventions/trace/rpc.yaml Show resolved Hide resolved
semantic_conventions/trace/rpc.yaml Show resolved Hide resolved
Comment on lines 97 to 100
- id: id
type: string
brief: "`id` property of request. Since protocol allows id to be int, string, `null` or missing (for notifications), value is expected to be cast to string for simplicity. Omit if this is a notification."
examples: ['10', 'request-7', '']
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this would be generic enough to be added on the parent-level (e.g., as rpc.call_id or rpc.request_id) so we don't have overlaps/duplicates later if this is added for other protocols/frameworks as well. @open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers any opinions?

semantic_conventions/trace/rpc.yaml Outdated Show resolved Hide resolved
@Rast1234 Rast1234 requested a review from a team April 26, 2021 20:04
@Rast1234
Copy link
Contributor Author

Fixed and replied where i could

Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

Thanks!
Please ignore the failing docfx check for now, it's just Chocolatey currently being down (https://status.chocolatey.org).

CHANGELOG.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/rpc.md Outdated Show resolved Hide resolved
semantic_conventions/trace/rpc.yaml Outdated Show resolved Hide resolved
semantic_conventions/trace/rpc.yaml Show resolved Hide resolved
@arminru arminru added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory labels Apr 27, 2021
Rast1234 and others added 2 commits April 30, 2021 12:08
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@carlosalberto carlosalberto merged commit 7275513 into open-telemetry:main May 7, 2021
arminru added a commit that referenced this pull request May 10, 2021
This entry slipped into the wrong section. The PR was merged after 1.3.0 was released.
bogdandrutu pushed a commit that referenced this pull request May 10, 2021
This entry slipped into the wrong section. The PR was merged after 1.3.0 was released.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please guide how to properly extend attributes for not-yet-documented protocols
4 participants