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

[receiver/elasticsearch]: add metrics for merge operations with total shard aggregation #14870

Merged

Conversation

aboguszewski-sumo
Copy link
Member

@aboguszewski-sumo aboguszewski-sumo commented Oct 11, 2022

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.

@djaglowski
Copy link
Member

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.

@djaglowski
Copy link
Member

Would you mind splitting off a PR for the one that was mistakenly omitted? This is a bug and should be accepted separately.

Nevermind. I missed that the metric was already be used. We're just adding another data point to it. Not a bug.

@aboguszewski-sumo aboguszewski-sumo force-pushed the elastic-add-merges branch 2 times, most recently from 01b3901 to e83e019 Compare October 21, 2022 11:03
@aboguszewski-sumo
Copy link
Member Author

I added the tests.

@aboguszewski-sumo aboguszewski-sumo force-pushed the elastic-add-merges branch 2 times, most recently from 61dbd1b to 19e3e53 Compare October 21, 2022 11:26
@aboguszewski-sumo
Copy link
Member Author

aboguszewski-sumo commented Oct 28, 2022

@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 full because it seemed the most suitable one). There are two reasons for doing so:

  1. This way we test the correctness of the metric, but also make sure that it is disabled by default.
  2. Rebases will be much easier to be done. To be honest, the most annoying thing in working on these metrics is modifying the files with test results (and in case of this receiver, there are 5 files in total). If only one test file has to be modified with each PR, the rebases can go more smoothly.

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 5 on main, not 4), so this is why this PR adds 3 metrics, but changes the number of errors by 4.

@djaglowski
Copy link
Member

@aboguszewski-sumo, I agree with your reasoning on the test files. This should be a good pattern to follow going forward.

@djaglowski djaglowski merged commit fa17377 into open-telemetry:main Oct 28, 2022
shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2022
… shard aggregation (open-telemetry#14870)

feat: add metrics for merge operations with total shard aggregation
@plantfansam plantfansam mentioned this pull request Jul 21, 2023
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.

None yet

2 participants