Skip to content

Conversation

@Amerousful
Copy link
Contributor

New flag to control list of available metrics due to reduce metric data size

--influxdb.metrics='mean,max'

@Amerousful
Copy link
Contributor Author

@soulgalore Hey!) This is a PR about we discussed several days ago.
I'm not sure that the solution is okay, and maybe there's a better way to do it...

@soulgalore
Copy link
Member

Looks good. I have two suggestions:

  1. What about another name? Metrics to me sounds like a metric like first paint or something else.
  2. Can you add a test case is https://github.com/sitespeedio/plugin-influxdb/blob/main/.github/workflows/linux.yml where the new option is used? That makes me comfortable that it will work over time :)

@Amerousful
Copy link
Contributor Author

  1. Totally agree with you about namings. I'll rework it.
  2. Sure.

@Amerousful
Copy link
Contributor Author

@soulgalore

  1. Changed to --influxdb.functions='mean,max'
  2. Add a test
  3. Fix the case when, e.g. p50 isn't included in passed functions and it writes as value
if (!availableFunctions.includes(end) && !functions.includes(end))

@Amerousful
Copy link
Contributor Author

Sorry, I have a concern about Linux. Should I add just another step with the filter by functions? And then it will be check manually in InfluxDB? I'm just worried that the previous step with all functions will mislead the check results.

@soulgalore
Copy link
Member

I didn't fully understand the concern about Linux? One thing you could do is in the "open" function https://github.com/sitespeedio/plugin-influxdb/blob/main/lib/index.js#L20 (that is called on startup) you could verify the input of influxdb.functions and if something is broken or wrong log or throw an error before the tests start?

@Amerousful
Copy link
Contributor Author

Copy that, I'll get it done

@Amerousful
Copy link
Contributor Author

@soulgalore Let's run the workflow)

@soulgalore soulgalore merged commit 0020017 into sitespeedio:main Apr 10, 2025
1 check passed
@soulgalore
Copy link
Member

Thank you @Amerousful , I'll release this first thing tomorrow.

@Amerousful
Copy link
Contributor Author

@soulgalore Thank you too) Deal.

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.

2 participants