Skip to content
This repository has been archived by the owner on Jun 2, 2023. It is now read-only.

Fixing issue #1: Metrics coming from plugin with compound name are not usable in thresholds #2

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

Conversation

aymeric-cr
Copy link

I open the PR to fix issue #1. I propose to use a function to sanitize metrics name, this function replace - sign by _ to avoid compileExpression function to interpret these signs as subtraction operators.

This PR fixes the problem for thresholds but it still persists for conditions which probably need the use of placeholders to by fixed.

I also add a .gitignore file to make easier the future work of contributors

…mpound plugin name using '-'

Add .gitignore file to ignore node_modules directory
@hassy
Copy link
Member

hassy commented Mar 17, 2023

Hi @aymeric-cr 👋 apologies for missing the PR back when you opened it.

This change would mean that the metric name that the user sees in the report is different from the metric name they would have to use in an ensure expression. That would be very confusing. Not being able to use metrics with dashes in their names is a real problem though, but we should solve it in a different way in order to maintain 1:1 mapping between metric names shown in the report, and used in ensure expressions.

@jfornfeist
Copy link

jfornfeist commented Apr 14, 2023

Hi @hassy, any news about this? I 'm stuck with the same issue.

How about defining a metric namespace in the other plugin(s) then. E.g. like this:

config:
  plugins:
    ensure: {}
    metrics-by-endpoint:
      useOnlyRequestNames: true
      metricsNamespace: "latency_endpoints"
  ensure:
    thresholds:
      - "latency_endpoints.response_time.some_endpoint.p95": 100

I created a PR for it over there.

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

Successfully merging this pull request may close these issues.

3 participants