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

[RenderTextFormat] Allow value errors to be rendered as comments #142

Merged
merged 2 commits into from
Dec 20, 2023

Conversation

joec4i
Copy link
Contributor

@joec4i joec4i commented Dec 20, 2023

This PR addresses a specific scenario encountered with Redis and RedisNg storage, where samples with mismatching labels can be stored. For instance:

127.0.0.1:6379> hgetall PROMETHEUS_:counter:foo_bar
1) "[\"bob\",\"alice\",\"eve\"]"
2) "1"
3) "__meta"
4) "{\"name\":\"foo_bar\",\"help\":\"counter-help-text\",\"type\":\"counter\",\"labelNames\":[\"label1\",\"label2\"]}"
5) "[\"bob\",\"alice\"]"
6) "1"

In the above example, the counter metadata specifies label1 and label2 but it's possible to store samples with more or fewer labels, e.g.["bob","alice","eve"]. This could happen under different circumstances. For example, a new version of the code introduces label3 for the counter.

Depending on the setup, flushing the storage after the new code is out could be a valid way to avoid the problem. However, it becomes complex in environments with canary deployment where multiple versions of the code are required to run together.

While there are ways to avoid the scenario, offering users the option to ignore errors for a limited number of metrics can be beneficial. It avoids the complete failure of the rendering operation in cases of minor inconsistencies. This PR does that.

Thanks for reviewing this! 🙏

Redis and RedisNg allow samples with mismatching labels to be stored, which could cause ValueError to be thrown when
rendering. Rendering would fail as a result, which is not ideal.

- Change render() to accept an optional $silent parameter. When set to true, render the errors as comments instead of
  throwing them and failing the whole operation.

Signed-off-by: Joe Cai <[email protected]>
@joec4i joec4i force-pushed the renderer_value_error branch from 76a8777 to dc0e6ce Compare December 20, 2023 06:16
Copy link
Member

@LKaemmerling LKaemmerling left a comment

Choose a reason for hiding this comment

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

Great idea! Just a really small blank line issue :) Great job!


$renderer = new RenderTextFormat();
$renderer->render($registry->getMetricFamilySamples());

Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this blank line?

Copy link
Member

@LKaemmerling LKaemmerling left a comment

Choose a reason for hiding this comment

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

I removed the line :) Again thank you, a great idea & great job!

@LKaemmerling LKaemmerling merged commit ad2a85f into PromPHP:main Dec 20, 2023
31 checks passed
@LKaemmerling
Copy link
Member

And released as v2.9.0 :) I guess this is a new world record, 9 min from opening of the MR, till merging & releasing :)

@joec4i
Copy link
Contributor Author

joec4i commented Dec 20, 2023

🤣 Thanks @LKaemmerling !

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