-
Notifications
You must be signed in to change notification settings - Fork 525
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
feat: store well defined metrics as times-series data streams #9730
Conversation
This pull request does not have a backport label. Could you fix it @kruskall? 🙏
NOTE: |
📚 Go benchmark reportDiff with the
report generated with https://pkg.go.dev/golang.org/x/perf/cmd/benchstat |
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.
My understanding is that the index_mode
needs to be set per index template, and the fields are used to create dimensions. We need to look into which fields should be used for a dimension, and which metrics should have a time_series_metric
definition.
Other things such as look-ahead time
might also need to be considered.
This is supposed to be a PoC and we need to test implications on the APM UI. Did you mean to create this PR to be ready for review? I suggest to put it into draft until everything is figured out.
Thanks for sharing! 🙇 I think I misunderstood how this should work, I'll read up more docs about it and udpate the PR |
bb72beb
to
a9d756a
Compare
This pull request is now in conflicts. Could you fix it @kruskall? 🙏
|
Remove index.sort.field for internal metrics: illegal_argument_exception: [illegal_argument_exception] Reason: [index.mode=time_series] is incompatible with [index.sort.field]
a9d756a
to
e070f52
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.
@kruskall lmk once this should be reviewed again. I know that there is currently a Kibana blocker, but we also discussed focusing on the dimensions.
@kruskall we discussed that (almost) all of the fields that are part of the transaction metrics aggregation key should be part of the dimensions. I find following fields from the key missing in this PR:
Can you explain why you excluded these fields from the TSDB key? If in doubt, I'd start out with adding all the fields from the aggregation key to the dimensions and see if that causes any issues or performance issues. Since a goal of this PoC is not to merge the change to TSDB but evaluate potential issues on the UI or on performance, please update as soon as possible, so that the work on the evaluation can start. |
@simitt I was trying to test the changes out progressively, unfortunately rally takes is uploading the corpora which takes an absurd amount of time on slow connections. I've added all the dimensions but I'm unable to get some numbers at the moment. |
Closing this for now until the blocker with limiting the number of dimensions is closed. |
@simitt @salvatore-campagna @felixbarny now that elastic/elasticsearch#93564 is solved and the TSDB dimension limit is gone, is it possible to reopen this and bring TSBD-support to APM? |
Motivation/summary
Checklist
apmpackage
have been made)For functional changes, consider:
How to test these changes
Related issues
Closes #9649