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

Add new Nodes metrics that include the consumed/produced topics #1223

Merged
merged 3 commits into from
Jul 11, 2023

Conversation

javimb
Copy link
Contributor

@javimb javimb commented Apr 26, 2023

Overview

These changes are a second iteration of PR#1130.
Since a service could have multiple Kafka consumers and producers, and each one could connect to different clusters, we want to introduce these new metrics to understand the relationship between consumers/producers, topics and nodes.

Testing

Although Kafka3MessageTest is disabled because it's flaky on GHA, I added some more checks around the metrics I introduced and I checked that the tests pass locally.

Checks

✅ Are your contributions backwards compatible with relevant frameworks and APIs? Yes
✅ Does your code contain any breaking changes? No
✅ Does your code introduce any new dependencies? No

These changes are a second iteration of PR#1130.
Since a service could have multiple Kafka consumers and producers, and each one could connect to different clusters, we want to introduce these new metrics
to unerstand the relationship between consumers/producers, topics and nodes.
@CLAassistant
Copy link

CLAassistant commented Apr 26, 2023

CLA assistant check
All committers have signed the CLA.

@kford-newrelic
Copy link
Contributor

@javimb Thanks for the PR - we'll be reviewing!

@kford-newrelic kford-newrelic added the on-roadmap Issue has been added to our product roadmap and will be worked in the coming quarter label Apr 26, 2023
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@2af9777). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 3729dca differs from pull request most recent head 6417be3. Consider uploading reports for the commit 6417be3 to get more accurate results

@@           Coverage Diff           @@
##             main    #1223   +/-   ##
=======================================
  Coverage        ?   64.60%           
  Complexity      ?     9096           
=======================================
  Files           ?      834           
  Lines           ?    40024           
  Branches        ?     6071           
=======================================
  Hits            ?    25858           
  Misses          ?    11601           
  Partials        ?     2565           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@meiao
Copy link
Contributor

meiao commented Jul 11, 2023

I've converted a few Iterable.forEach calls with loops.
forEach calls will force an extra method call and the creation of a lambda.
These are usually negligible for an application, but we try to keep the agent as lean as possible.

There is another point of improvement which would be not to store both metric and event names, since only one of those is ever used.
This requires more code changes and possibly more CPU overhead. And the decrease in memory may not be meaningful. So just leaving this note here and in the future that can be revisited.

Testing is done on: https://github.com/newrelic/newrelic-java-agent/actions/runs/5524205746

@meiao meiao merged commit 529ac83 into newrelic:main Jul 11, 2023
6 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on-roadmap Issue has been added to our product roadmap and will be worked in the coming quarter
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants