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

Graphite metrics coverage #491

Merged
merged 10 commits into from Sep 17, 2019
Merged

Graphite metrics coverage #491

merged 10 commits into from Sep 17, 2019

Conversation

@ham1
Copy link
Contributor

@ham1 ham1 commented Sep 15, 2019

Description

Added TextGraphiteMetricsSenderSpec to provide some coverage of TextGraphiteMetricsSender and PickleGraphiteMetricsSender and PickleGraphiteMetricsSenderSpec. Also cleaned up code in the surrounding area.

Motivation and Context

More tests!

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
@ham1 ham1 force-pushed the graphite-metrics-coverage branch from 5fea03f to 7762594 Sep 15, 2019
@ham1
Copy link
Contributor Author

@ham1 ham1 commented Sep 15, 2019

I have some questions for everyone:

  1. I've removed the .clear() from the lists after sending because this didn't seem to be required as there will no longer be a reference to the list, therefore will be GC'd, this should also make sending slightly more efficient.
  2. I've added an overloaded setup method to TextGraphiteMetricsSender to enable testing, is there a better/preferred way? e.g. should it be protected or private or a new constructor?
  3. should destroy() also set metrics to null or are we relying on the original reference to be removed after destroy is called?

final String key = entry.getKey();
final SamplerMetric metric = entry.getValue();
if(key.equals(CUMULATED_METRICS)) {
getMetricsPerSampler().forEach((key, metric) -> {
Copy link
Contributor

@pmouawad pmouawad Sep 16, 2019

Choose a reason for hiding this comment

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

Did you check if use of lambda here would impact negatively performances ? (As per other PR discussion ?)

Thanks

@pmouawad
Copy link
Contributor

@pmouawad pmouawad commented Sep 16, 2019

Hi @ham1 ,
Thanks for this very useful PR.
Except for the forEach note, I am ok with code.
Could you check and either keep previous form or keep it if lambda is not counter performant.

Thanks

@pmouawad pmouawad merged commit ac7d2b3 into apache:master Sep 17, 2019
1 check passed
@ham1 ham1 deleted the graphite-metrics-coverage branch Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants