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

Differentiate running a function with test=True or mock=True #28

Closed
julioz opened this issue May 23, 2023 · 7 comments
Closed

Differentiate running a function with test=True or mock=True #28

julioz opened this issue May 23, 2023 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@julioz
Copy link

julioz commented May 23, 2023

When running a function in test mode (for example salt <target> state.highstate test=True), we get metrics reported as if the function was executed successfully/failing according to the result:

salt_expected_responses_total{function="state.highstate",state="highstate"} 1
# HELP salt_function_responses_total Total number of response per function processed
# TYPE salt_function_responses_total counter
salt_function_responses_total{function="state.highstate",state="highstate",success="false"} 1
# HELP salt_function_status Last function/state success, 0=Failed, 1=Success
# TYPE salt_function_status gauge
salt_function_status{function="state.highstate",minion="myminion.company.com",state="highstate"} 0
# HELP salt_new_job_total Total number of new job processed
# TYPE salt_new_job_total counter
salt_new_job_total{function="state.highstate",state="highstate",success="true"} 1
# HELP salt_responses_total Total number of response job processed
# TYPE salt_responses_total counter
salt_responses_total{minion="myminion.company.com",success="false"} 1

It would be benefitial to differentiate running states in test mode, mock mode or "production".

@kpetremann
Copy link
Owner

kpetremann commented May 23, 2023

Hello @julioz,

Thanks for your insights.

we get metrics reported as if the function was executed successfully/failing according to the result

This is by design, respecting Salt "success" result.
A failure in dry-run (test=True) is usually considered as an issue. I am not familiar with the usage of mock=True.

Why do you want to exclude or differentiate them? Can you elaborate on your usecase?

Note: adding more labels to identify dry-run from mock from prod will quickly increase the metric cardinality, especially when health-minion feature is enabled. So I would like to perfectly understand the need to find the most appropriate implementation.

@kpetremann kpetremann self-assigned this May 23, 2023
@kpetremann kpetremann added the question Further information is requested label May 23, 2023
@julioz
Copy link
Author

julioz commented May 23, 2023

Why do you want to exclude or differentiate them? Can you elaborate on your usecase?

While keeping track of success/failure on automatic job runs (e.g highstate) we still would like to give engineers to possibility to iterate on their Salt states while developing, while not poisoning our metric reporting, especially because we have alerts set for when failure rate is above a certain threshold.

So the use-case would be for an engineer iterating on a new state (or refactor) and potentially using test=True to dry-run states from the salt-master node.

Note: adding more labels to identify dry-run from mock from prod will quickly increase the metric cardinality, especially when health-minion feature is enabled. So I would like to perfectly understand the need to find the most appropriate implementation.

Totally understand - what I was thinking of was some sort of flag we can pass to the exporter so that it ignores dry-run executions, or alternatively expose a separate metric with potentially less granularity (i.e no minion labels, etc) counting how many dry-run executions were read from the event bus.

In other words, passing that flag would allow for us to split what's a dry-run and what's not, in terms of success/failed state executions.

@kpetremann
Copy link
Owner

kpetremann commented May 23, 2023

Thanks for the details, crystal clear.

I had not thought about this usecase because we do dev on dev/preprod instances, so production metrics are never poisoned by development.

I'll probably go for an option to ignore dry-run and/or mock.
FYI, I am working on a revamp of the flags to simplify the configuration of the exporter.

To be transparent, I won't probably be able to work on this until beginning of June as I am on vacation :)
But I'll keep you updated as soon as I have made progress.

@kpetremann kpetremann added enhancement New feature or request and removed question Further information is requested labels May 23, 2023
@julioz
Copy link
Author

julioz commented May 23, 2023

Sounds great, let me know when you work on it and I'm happy to review the proposed metric. Thanks for the help!

@kpetremann
Copy link
Owner

hi @julioz,

I have added the option to ignore test and mock. See the dry_run_mock_exclusion branch.

It is not merged yet as I am working on a rework of the configuration system of the exporter.

@julioz
Copy link
Author

julioz commented Jun 22, 2023

Awesome, thank you very much. Looking forward to trying this out when the release is ready 👏

@kpetremann
Copy link
Owner

Hi @julioz,

The feature is included in the new v0.6.0 release! 🎉

@julioz julioz closed this as completed Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants