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

Move export prometheus_text_format:escape_label_value outside of TEST def #158

Merged

Conversation

aaron-seo
Copy link
Contributor

prometheus_text_format:escape_label_value is a useful helper function that users can benefit from using without having to duplicate the function in their code.

Namely, it can be used to escape prometheus core metric label values -- based on issue rabbitmq/rabbitmq-server#9648 and relevant PR/review from rabbitmq/rabbitmq-server#9656

@@ -76,6 +77,15 @@ format(Registry) ->
ok = file:close(Fd),
Str.

-spec escape_label_value(binary() | iolist() | undefined) -> binary().
Copy link
Contributor

Choose a reason for hiding this comment

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

Existing type-spec is a bit strange, this function does not return binary() for input undefined

Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to fix in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like my local workspace didn't have GPG signatures setup correctly, so I'll create a new PR anyways with the signed commits. I'll update the spec to the following to more accurately represent the function.

-spec escape_label_value(binary() | iolist()) -> binary()
                      ; (undefined) -> no_return().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nvm no need for new PR, figured out how to retroactively sign commits :) Otherwise, the spec has been updated, please let me know if you find it more appropriate. Thanks in advance

@aaron-seo aaron-seo force-pushed the export_escape_label_value branch 2 times, most recently from c2bc355 to e3f2c8d Compare October 16, 2023 19:40
@aaron-seo aaron-seo force-pushed the export_escape_label_value branch from 38b00b8 to f28837a Compare October 16, 2023 20:07
@michaelklishin
Copy link
Contributor

@ikvmw you may have to explicitly approve workflow runs for this PR as it is from/by a first-time contributor

@aaron-seo
Copy link
Contributor Author

May need to be merged manually even after approval, due to the job continuous-integration/travis-ci Expected — Waiting for status to be reported stalling. @ikvmw @deadtrickster Thanks in advance :)

@aaron-seo
Copy link
Contributor Author

Just bumping this for merge, if there are other tasks needed for the process, please feel free to let me know, thanks @deadtrickster @ikvmw

@deadtrickster deadtrickster merged commit 297a6e6 into deadtrickster:master Nov 16, 2023
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.

5 participants