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

Provide semantic conventions for RPC metrics #1162

Merged
merged 5 commits into from
Nov 26, 2020

Conversation

jsuereth
Copy link
Contributor

Fixes #1016

Changes

Adds semantic conventions for RPC metrics.

  • Re-use existing RPC attribute spec YAML. Note: this interacts oddly pulling from trace => metrics.
  • Define 3 (x 2) key metrics for RPC: rpc.?.duration, rpc.?.request_size and rpc.?.response_size, crossed by server or client
  • Add warning around reducing cardinality in attributes for metrics (cross-reference the same information for spans).

Deviations from OpenCensus

  • This does not track a "count" of messages that have been issued.
  • This does not pull different metrics per-method vs. per-rpc, aggregation across labels is assumed.

Copy link
Member

@justinfoote justinfoote left a comment

Choose a reason for hiding this comment

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

This looks good to me.
My only question is: should the rpc metrics also include HTTP labels if the RPC calls are happening over HTTP?

@jsuereth
Copy link
Contributor Author

@justinfoote Regarding RPC having HTTP labels if it's over HTTP. While Personally I could see reasons to include it or not, I think perhaps we should follow from TRACE: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/rpc.md#distinction-from-http-spans

TL;DR: The metrics is more about the fact that it's an RPC then it is about HTTP GET/POST/PUT/DELETE resources, so we shouldn't cross-report.

@github-actions
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 Nov 19, 2020
@jsuereth
Copy link
Contributor Author

@MrAlias Is there anything else I need to do for this PR?

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@github-actions github-actions bot removed the Stale label Nov 20, 2020
@jsuereth
Copy link
Contributor Author

jsuereth commented Nov 24, 2020

@carlosalberto What else do I need to do for this PR?

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.

Please add an entry in CHANGELOG.md for your new semantic convention. Thanks!

specification/metrics/semantic_conventions/rpc.md Outdated Show resolved Hide resolved
specification/metrics/semantic_conventions/rpc.md Outdated Show resolved Hide resolved
@arminru
Copy link
Member

arminru commented Nov 24, 2020

@jsuereth The build checks required to pass for merging are currently saying "Waiting for status to be reported" because we switched to Github actions in the meantime. If you merge or rebase onto the current master, those changes should be picked up and the build should pass.

@jsuereth
Copy link
Contributor Author

Please add an entry in CHANGELOG.md for your new semantic convention. Thanks!

Done!

@arminru arminru merged commit a519397 into open-telemetry:master Nov 26, 2020
@jsuereth jsuereth deleted the wip-rpc-metric-conventions branch April 17, 2021 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define metric semantic conventions for RPC
7 participants