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

feat: Support to exclude default metrics and other options #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yorch
Copy link
Contributor

@yorch yorch commented Mar 8, 2021

  • Adds httpMetricsPrefix option to optionally add a prefix to HTTP metrics.
  • Adds excludeDefaultMetricLabels option to exclude all or certain metrics that are added by default.
  • Adds useCountersForRequestSizeMetric option to expose two counters for Request Size (_sum and _count) instead of Histogram.
  • Adds useCountersForResponseSizeMetric option to expose two counters for Response Size (_sum and _count) instead of Histogram.

@yorch
Copy link
Contributor Author

yorch commented Mar 8, 2021

@kobik This is another PR for changes I've made for my company's use case, I'm fixing tests and making sure the build passes, but if you could start taking a look and let me know your feedback please. Thanks

@yorch yorch changed the title (WIP) Support for exclude default metrics Support to exclude default metrics and other options Mar 9, 2021
@yorch yorch changed the title Support to exclude default metrics and other options feat: Support to exclude default metrics and other options Mar 9, 2021
@kobik
Copy link
Collaborator

kobik commented Mar 9, 2021

@yorch can you please rebase from master so we'll have the circleci running on your branch?

@yorch yorch force-pushed the exclude-metrics branch 2 times, most recently from d758f74 to b068881 Compare March 9, 2021 14:27
@yorch
Copy link
Contributor Author

yorch commented Mar 14, 2021

@kobik this is ready for review please

@kobik
Copy link
Collaborator

kobik commented Mar 17, 2021

@kobik this is ready for review please

Will do

@kobik
Copy link
Collaborator

kobik commented Apr 11, 2021

Hi @yorch , sorry for the delay it's been busy time + holidays here.

I started reviewing you PR and I have a question regarding httpMetricsPrefix, can you share your use-case?

@yorch
Copy link
Contributor Author

yorch commented Apr 13, 2021

@kobik The use case for the prefix is that depending on your infrastructure, you may want to avoid having multiple different NodeJS services exposing the same metrics. Like in our case, we have a big variety of NodeJS services, so we only want similar services to share metric names.

@kobik
Copy link
Collaborator

kobik commented Apr 22, 2021

ok, so i have a little different idea in mind.

how about adding excludeDefaultMetricLabels as you did in order not break anything and on top of that add the option to selectively pick the metrics you'd like to expose?

and on the next breaking version we can change the default behavior to make it more consistent, making excludeDefaultMetricLabels false by default and httpRequestMetrics and httpResponseMetrics to be required.

this way the developer will be aware of the metrics he's adding.

i.e

{
   excludeDefaultMetricLabels: true,
   httpRequestMetrics: ["sum", "count", "histogram"],
   httpResponseMetrics: ["sum", "count", "histogram"]
}

let me know what you think.

@pragmaticivan
Copy link

Are there plans to merge this PR?

@kobik
Copy link
Collaborator

kobik commented Jun 28, 2021

never got a response from @yorch regarding my latest comment.

i don't want to introduce a breaking change if we can avoid that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants