Skip to content

Conversation

@mcollina
Copy link
Member

@mcollina mcollina commented Mar 1, 2025

Alternative implementation of platformatic/platformatic#3883.

Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
index.js Outdated
return false
}

let needEntrySummary = true
Copy link
Member

Choose a reason for hiding this comment

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

I would make this optional (disabled by default.)

index.js Outdated
help: 'request duration in seconds summary',
labelNames,
registers,
collect: function () {
Copy link
Member

Choose a reason for hiding this comment

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

This will save an "empty" metric every time the prom fetches metrics (every 1s in most cases). Are you sure you need to push a new metric every second and not just save it once after summary/histogram initialisation?

something like summary.observe(0)

index.js Outdated
help: 'request duration in seconds',
labelNames,
registers,
collect: function () {
Copy link
Member

Choose a reason for hiding this comment

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

index.js Outdated
if (needEntrySummary) {
const summaryTimer = this.startTimer()
summaryTimer({
method: 'GET',
Copy link
Member

Choose a reason for hiding this comment

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

Tiny problem with that solution is that it will work only if you aggregate values when you query the prom metrics. For example if I want to get metrics for all POST requests or all requests with status code 200 they will not have this default values as they stored with labels "GET/__empty_metrics/404".

Obviously we don't know all route labels, but maybe it will make sense to store such metrics for some method + status_code combinations.

Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina
Copy link
Member Author

mcollina commented Mar 3, 2025

@ivan-tymoshenko PTAL

Copy link

@MzUgM MzUgM left a comment

Choose a reason for hiding this comment

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

Looks good to me

@MzUgM MzUgM requested a review from ivan-tymoshenko March 4, 2025 17:38
Copy link
Member

@ivan-tymoshenko ivan-tymoshenko left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit e0d4c62 into main Mar 6, 2025
6 checks passed
MzUgM added a commit that referenced this pull request Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants