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

No prometheus metrics #90

Closed
fcamposdespegar opened this issue Jun 14, 2023 · 6 comments
Closed

No prometheus metrics #90

fcamposdespegar opened this issue Jun 14, 2023 · 6 comments

Comments

@fcamposdespegar
Copy link

With last changes we lost prometheus metrics. Problem may be related with metric names.

@puneetkhanduri
Copy link
Collaborator

Users were running into this issue with long metric names so we had to abbreviate and trim the names to avoid exceptions. The metrics are now being exported with abbreviated names as a stopgap solution which we don't like. Open to suggestions.

@suever
Copy link

suever commented Jun 16, 2023

@puneetkhanduri It seems as though the use of Prometheus labels could be beneficial to shorten the metric names but maintain legibility. Not only is the current approach difficult to read (and is not documented at all), it seems to combine values that should really be separated out (e.g. all GET requests get squashed into the same metric so you cannot filter based on path).

@puneetkhanduri
Copy link
Collaborator

puneetkhanduri commented Jun 16, 2023

@suever I can see the counter interface offers us the ability to provide tags in addition to metric names. Can you propose a naming structure until the long metric names issue is resolved. Here's what I currently have in mind:

name = <unmodified full original metric name>.take(63) // take first 63 characters and drop the rest
tags = "name", <unmodified full original metric name>

or

name = <unabbreviated endpoint>.<abbreviated field>
tags = "type" , ("raw" | "noise")

Open to anything better you might have in mind.

@puneetkhanduri
Copy link
Collaborator

@fcamposdespegar @suever : Please share your feedback if any so we may address and close this issue.

@suever
Copy link

suever commented Jun 28, 2023

Sorry for the delay, I had to actually install an old version of opendiffy to determine what the metrics were supposed to represent based on the names before they were truncated.

I would propose something like the following where the cardinality will still be high, but the number of metrics (and length of their names) is reasonable.

These examples may not be 100% accurate but provide an example of my thinking

diffy_requests_total{method="GET", path="/app"}
diffy_requests_differences_total{method="GET", path="/app", exclude=noise="true"}
diffy_requests_differences_total{method="GET", path="/app", exclude=noise="false"}
diffy_requests_differences_total{method="GET", path="/app", exclude_noise="true", key="headers"}
diffy_requests_differences_total{method="GET", path="/app", exclude_noise="true", key="body.field"}
diffy_requests_differences_total{method="GET", path="/app", exclude_noise="true", key="headers.accept-encoding"}

@puneetkhanduri
Copy link
Collaborator

Fixed in the latest release with the introduction of endpoint and field tags to corresponding metrics.

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

No branches or pull requests

3 participants