-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[receiver/elasticsearch]: add metrics for merge operations with total shard aggregation #14870
[receiver/elasticsearch]: add metrics for merge operations with total shard aggregation #14870
Conversation
b2fcc00
to
f05e831
Compare
Would you mind splitting off a PR for the one that was mistakenly omitted? This is a bug and should be accepted separately. The two new metrics seems reasonable to me. |
Nevermind. I missed that the metric was already be used. We're just adding another data point to it. Not a bug. |
01b3901
to
e83e019
Compare
I added the tests. |
61dbd1b
to
19e3e53
Compare
19e3e53
to
02c1724
Compare
02c1724
to
59da8d0
Compare
@djaglowski I changed the metrics to disabled by default. There is one thing I've changed in the tests due to that. I decided that it would be the best if these metrics were enabled only in one test (I picked
I'd like to follow this convention for other metrics that are going to be disabled by default (I'll be fixing the PRs gradually), so if you have any objection, please raise it now. Also, one small thing: the recently merged PR did not change the number of errors emitted (it should be |
@aboguszewski-sumo, I agree with your reasoning on the test files. This should be a good pattern to follow going forward. |
… shard aggregation (open-telemetry#14870) feat: add metrics for merge operations with total shard aggregation
Description:
Three new metrics have been added. One was already present in
metadata.yaml
, but was not emitted.Link to tracking Issue: #14635
Testing:
Unit and integration
Documentation:
Generated with
mdatagen
.