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

[collectd 6] Allow arbitrary UTF-8 strings as label names. #4212

Merged
merged 18 commits into from
Dec 28, 2023

Conversation

octo
Copy link
Member

@octo octo commented Dec 22, 2023

Source: https://opentelemetry.io/docs/specs/semconv/general/attribute-naming/

ChangeLog: Daemon: All valid UTF-8 strings are now accepted as resource attribute name and metric label name.

@octo octo requested review from a team as code owners December 22, 2023 21:22
@collectd-bot collectd-bot added this to the 6.0 milestone Dec 22, 2023
Copy link
Contributor

@eero-t eero-t left a comment

Choose a reason for hiding this comment

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

I'd like test(s) checking also handling of extra (closing) curly brace(s). Otherwise looks OK on quick look.

@octo
Copy link
Member Author

octo commented Dec 28, 2023

I'd like test(s) checking also handling of extra (closing) curly brace(s). Otherwise looks OK on quick look.

Done.

@octo
Copy link
Member Author

octo commented Dec 28, 2023

Rebased to resolve a merge conflict.

Copy link
Contributor

@eero-t eero-t left a comment

Choose a reason for hiding this comment

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

Approved (tests being added and them passing makes this easier!)

Comment on lines -38 to +48
#define VALID_LABEL_CHARS \
/* If these characters are used in resource attribute names or metric label
* names, they will not cause quotes to be printed when formatting the metric
* name. Resource attribute values and metric label values are always printed in
* quotes. */
#define UNQUOTED_LABEL_CHARS \
"abcdefghijklmnopqrstuvwxyz" \
"ABCDEFGHIJKLMNOPQRSTUVWXYZ" \
"0123456789_.-"
"0123456789_.-:"

/* Metric names must match the regex `[a-zA-Z_:][a-zA-Z0-9_:]*` */
// instrument-name = ALPHA 0*254 ("_" / "." / "-" / "/" / ALPHA / DIGIT)
#define VALID_NAME_CHARS VALID_LABEL_CHARS "/"
#define VALID_NAME_CHARS \
"abcdefghijklmnopqrstuvwxyz" \
"ABCDEFGHIJKLMNOPQRSTUVWXYZ" \
"0123456789_.-/"
Copy link
Contributor

Choose a reason for hiding this comment

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

After git grep abcdefghijklmnopqrstuvwxyz, I wonder whether it would make sense to add ALNUM_CHARS define that would be used by both of these:

#define UNQUOTED_LABEL_CHARS  ALNUM_CHARS "_.-:"
#define VALID_NAME_CHARS      ALNUM_CHARS "_.-/"

And also in:

  • write_prometheus.c: VALID_LABEL_CHARS / VALID_NAME_CHARS
  • format_stackdriver.c: metric_type()::valid_chars

But that could be another PR.

@octo octo merged commit bbdae82 into collectd:collectd-6.0 Dec 28, 2023
23 checks passed
@octo octo deleted the 6/names branch December 28, 2023 19:49
@octo octo added the Feature label Jan 12, 2024
@octo octo added the core label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants