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 for metrics custom labels #57

Merged
merged 27 commits into from Mar 8, 2021
Merged

Conversation

yorch
Copy link
Contributor

@yorch yorch commented Sep 20, 2020

  • Provides support for custom / extra labels for each of the http_* metrics.

Example

promApiMetrics({
    metricsAdditionalLabels: ['user_group', 'role'],
    getMetricsAdditionalLabelValues: (req: Request) => {
        const { userGroup, role } = req.context;
        return {
            ...(userGroup ? { user_group: userGroup } : {}),
            ...(role ? { role } : {})
        };
    }
})

Result:

http_request_duration_seconds_bucket{le="0.1",method="POST",route="/path",code="200",user_group="admin",role="superadmin"} 0
http_request_duration_seconds_bucket{le="0.5",method="POST",route="/path",code="200",user_group="admin",role="superadmin"} 0
http_request_duration_seconds_bucket{le="1",method="POST",route="/path",code="200",user_group="admin",role="superadmin"} 0
http_request_duration_seconds_bucket{le="5",method="POST",route="/path",code="200",user_group="admin",role="superadmin"} 2
http_request_duration_seconds_bucket{le="+Inf",method="POST",route="/path",code="200",user_group="admin",role="superadmin"} 2
http_request_duration_seconds_sum{method="POST",route="/path",code="200",user_group="admin",role="superadmin"} 4.657347321
http_request_duration_seconds_count{method="POST",route="/path",code="200",user_group="admin",role="superadmin"} 2

@yorch
Copy link
Contributor Author

yorch commented Sep 20, 2020

Not a big fan of the new properties names (metricsExtraLabels, getMetricsExtraLabelValues), so open to suggestions

Copy link
Collaborator

@kobik kobik left a comment

Choose a reason for hiding this comment

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

Thanks for your PR @yorch , much appreciated!

In addition to my comments, we're missing tests for custom labels.

Can you please add the relevant test cases?

src/metrics-middleware.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/express-middleware.js Outdated Show resolved Hide resolved
src/koa-middleware.js Outdated Show resolved Hide resolved
src/metrics-middleware.js Outdated Show resolved Hide resolved
@yorch
Copy link
Contributor Author

yorch commented Oct 5, 2020

@kobik thanks for the review! Addressed the feedback:

  • Added input validation
  • Added more tests to cover the new options

@yorch
Copy link
Contributor Author

yorch commented Oct 12, 2020

@kobik let me know if you have any other feedback please! I have a few other changes on my fork that would like to put PRs for.

@kobik
Copy link
Collaborator

kobik commented Oct 15, 2020

Thanks @yorch for your work and nice input validation addition!

But you you have created a problem (good one 🙂 ), I meant to add validation only for the new props as adding validation for the older props could be a breaking change.

I'm considering whether to release a new major or wait for more breaking stuff and for now remove the validation from the old props.

README.md Outdated Show resolved Hide resolved
@kobik
Copy link
Collaborator

kobik commented Feb 9, 2021

Hi @yorch, is this still relevant?

@jbarnaby-medallia
Copy link

@kobik sorry for the delay, please let me know if you have any more feedback before this can be merged. thanks!

@yorch
Copy link
Contributor Author

yorch commented Feb 16, 2021

@kobik had to update package-lock.json as it seems like in the last commit in master, your internal NPM server URL was committed so builds for PRs are failing.

@yorch
Copy link
Contributor Author

yorch commented Feb 27, 2021

@kobik friendly reminder this is ready :)

@kobik
Copy link
Collaborator

kobik commented Feb 28, 2021

@yorch on it

Copy link
Collaborator

@kobik kobik left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, having loaded with work and i wanted to review it again as i didn't remember the changes

README.md Outdated Show resolved Hide resolved
src/metrics-middleware.js Show resolved Hide resolved
@kobik
Copy link
Collaborator

kobik commented Mar 4, 2021

@yorch one last thing is adding it to the changelog 🙂

@yorch
Copy link
Contributor Author

yorch commented Mar 4, 2021

@kobik if I change the CHANGELOG, there are gonna be conflicts with the other PR, so I'll have to resolve conflicts after one of them merges. Why not making those changelog in a different commit?

@kobik
Copy link
Collaborator

kobik commented Mar 4, 2021

@yorch you can now rebase

@yorch
Copy link
Contributor Author

yorch commented Mar 4, 2021

@kobik done

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@kobik kobik left a comment

Choose a reason for hiding this comment

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

thanks @yorch, much appreciated!

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