Skip to content

Make the metric set limit configurable (#2147)#2148

Merged
eyalkoren merged 16 commits intoelastic:mainfrom
tobiasstadler:fix-2147
Jul 4, 2022
Merged

Make the metric set limit configurable (#2147)#2148
eyalkoren merged 16 commits intoelastic:mainfrom
tobiasstadler:fix-2147

Conversation

@tobiasstadler
Copy link
Contributor

@tobiasstadler tobiasstadler commented Sep 22, 2021 •
edited by eyalkoren
Loading

What does this PR do?

Fixes #2147

Checklist

  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation
@github-actions github-actions bot added agent-java community Issues and PRs created by the community triage labels Sep 22, 2021
@tobiasstadler
Copy link
Contributor Author

tobiasstadler commented Sep 22, 2021

I did not add this feature to the changeling/documentation because metric_set_limit is a internal option.
@ghost
Copy link

ghost commented Sep 22, 2021 •
edited by ghost
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-07-04T07:06:48.019+0000

  • Duration: 50 min 35 sec

Test stats 🧪

Test Results
Failed 0
Passed 3053
Skipped 36
Total 3089

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark tests.

  • run jdk compatibility tests : Run the JDK Compatibility tests.

  • run integration tests : Run the Agent Integration tests.

  • run end-to-end tests : Run the APM-ITs.

  • run windows tests : Build & tests on windows.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)
@SylvainJuge SylvainJuge added the size:small Small (S) tasks label Jan 31, 2022
@tobiasstadler
Copy link
Contributor Author

tobiasstadler commented Feb 3, 2022

@felixbarny What is your opinion?
@tobiasstadler
Copy link
Contributor Author

tobiasstadler commented Mar 30, 2022

ping
2 similar comments
@tobiasstadler
Copy link
Contributor Author

tobiasstadler commented May 4, 2022

ping
@tobiasstadler
Copy link
Contributor Author

tobiasstadler commented Jun 29, 2022

ping
eyalkoren
eyalkoren reviewed Jul 3, 2022
View reviewed changes
apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/MetricRegistry.java Outdated
return metricSet;
}
if (activeMetricSets.size() < METRIC_SET_LIMIT) {
if (activeMetricSets.size() < coreConfiguration.getMetricSetLimit()) {
Copy link
Contributor

@eyalkoren eyalkoren Jul 3, 2022

Choose a reason for hiding this comment

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

This is a bit misleading as it means that the limit can be dynamically updated. However, since metricsets are typically registered only once per startup, modifying this at runtime will only take affect on not-yet registered metrics.
Please keep the limit as a final field initialized through construction, so that it's more obvious and predictive.
@eyalkoren
Copy link
Contributor

eyalkoren commented Jul 3, 2022

I did not add this feature to the changeling/documentation because metric_set_limit is a internal option.

It should be added to the CHANGELOG, but no need to document it indeed.
@eyalkoren
Copy link
Contributor

eyalkoren commented Jul 3, 2022

run integration tests
eyalkoren
eyalkoren approved these changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

Thanks!
@tobiasstadler @eyalkoren
Update apm-agent-core/src/main/java/co/elastic/apm/agent/metrics/Metr…
0f7b4e0 
…icRegistry.java

Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com>
@eyalkoren
Copy link
Contributor

eyalkoren commented Jul 4, 2022

run elasticsearch-ci/docs
@eyalkoren eyalkoren merged commit 32ee207 into elastic:main Jul 4, 2022
@tobiasstadler tobiasstadler deleted the fix-2147 branch July 4, 2022 10:34
@tobiasstadler
Copy link
Contributor Author

tobiasstadler commented Jul 4, 2022

Thank You!
@mattiaspantzare
Copy link

mattiaspantzare commented Jul 13, 2022

If this is documented in the changelog it is in fact documented, the changelog is part of the documentation.

If it is an "internal option" (whatever that is), why is there an option for it? If the user is not supposed to set the option, who is?

This can't be a fix for #2147 if the user that need to set it can't find it.

If it is unsupported it can't be fix for a user bug.

Please document this properly.
@jackshirazi
Copy link
Contributor

jackshirazi commented Jul 19, 2022

Yes you are right. "internal" doesn't actually mean it's not for users - the agent is fully open sourced so anyone can see what is there and use any option.

"internal" is shorthand for "this option is an edge case that affects very few users but enough for us to add it, but we're not sure it's helpful to add it in the public documentation because too many rarely needed options makes for extra confusion"

But I agree in this case it's better to document it in the public documentation, and will do so. Thank you for your feedback!
@jackshirazi jackshirazi mentioned this pull request Jul 19, 2022
Merged
@jackshirazi jackshirazi mentioned this pull request Aug 9, 2022
Open
@kelunik kelunik mentioned this pull request Oct 10, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-java community Issues and PRs created by the community size:small Small (S) tasks triage

5 participants

@tobiasstadler @eyalkoren @mattiaspantzare @jackshirazi @SylvainJuge