-
Notifications
You must be signed in to change notification settings - Fork 116
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
docs/agents: add sampling spec #307
Conversation
* Add section about tracestate * Update docs/agents/sampling.md * Update docs/agents/distributed-tracing.md Co-authored-by: Andrew Wilkins <[email protected]>
af072f0
to
a3e3eaf
Compare
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.
Thanks @axw , regarding the propagation of the sampling rate via the tracestate
header, I think it adds over head for the users since the tracestate
needs to be configured for CORS. Furthermore, it should be
considered a breaking change for the RUM agent since existing setups will break once we add this header to requests.
- rename "elastic" to "es" - add note about validating tracestate
@jahtalab I don't see any alternative other than sending another header. I suppose it'll have to be opt-in for RUM. That means any traces originating from RUM will not have service map edge metrics unless the feature is enabled. |
Co-authored-by: Felix Barnsteiner <[email protected]>
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.
Looks good to me!
We've got all the required approvals now. This is scheduled to be merged next Monday unless there are objections. |
The sampling rate will be used by the server for scaling transaction and span metrics. | ||
|
||
Transaction metrics will be used by the UI to display transaction distributions and throughput, | ||
from the perspective of the transaction's service (grouped by `service.name` and `transaction.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.
@axw Probably no need to add this to the agents spec, more out of curiosity, what other dimensions are you planning to add? outcome
? result
? What's the cardinality limit for transaction.name
? Are you planning to also track metrics without the transaction.name
dimension to speed up queries that go across all transaction names? Could also be a special-case name like transaction.name: "_all"
.
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 are the current plans regarding the detection of dynamic parts in the URL?
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.
How important is it that all agents report ${request.method} unknown route
instead of ${request.method} ${request.path}
for the metrics collection to work properly? Is the metrics collection also scheduled for 7.10?
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.
@axw Probably no need to add this to the agents spec, more out of curiosity, what other dimensions are you planning to add? outcome? result?
result
is already in there, and outcome
will be added once we've finalised the spec. The full list is at https://github.com/elastic/apm-server/blob/5cb4101d705effdf2f54e2e45847ee92033e806e/x-pack/apm-server/aggregation/txmetrics/aggregator.go#L414
What's the cardinality limit for transaction.name?
The server has a configurable limit for the number of transaction metric buckets, which resets after every reporting interval. It's currently defaulting to 1000. Once that limit is reached, the server will start recording single-value metrics for new buckets.
Are you planning to also track metrics without the transaction.name dimension to speed up queries that go across all transaction names? Could also be a special-case name like transaction.name: "_all".
Not currently planning to do this in the server, but the UI might create transforms based off the transaction group metrics: elastic/kibana#74498
What are the current plans regarding the detection of dynamic parts in the URL?
None that I'm aware of. RUM recently introduced a heuristic-based approach: elastic/apm-agent-rum-js#827
How important is it that all agents report ${request.method} unknown route instead of ${request.method} ${request.path} for the metrics collection to work properly? Is the metrics collection also scheduled for 7.10?
Very important. If transaction names have such high cardinality, then distributions become less meaningful. I believe we're intending to preview the Metrics-based UI in 7.10.
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.
I've added some remarks to the Kibana issue: elastic/kibana#74498 (comment)
Very important. If transaction names have such high cardinality, then distributions become less meaningful. I believe we're intending to preview the Metrics-based UI in 7.10.
That's good info, I'll work with the agent teams to ensure we're aligned.
💔 Build FailedExpand to view the summary
Build stats
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
Create a more detailed spec for sampling, including the new requirement to capture the sampling rate in transactions and spans.
Supersedes #270