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

Remove trailing comma from labels in prometheus encoding #62

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

erikvanoosten
Copy link
Contributor

Here is an example line from the prometheus encoding:

summary{error="0.03",quantile="0.1",} 138.0625

According to the prometheus text based format the trailing comma should not be there.

Here is an example line from the prometheus encoding:
```
summary{error="0.03",quantile="0.1",} 138.0625
```

According to the [prometheus text based format](https://prometheus.io/docs/instrumenting/exposition_formats/#text-based-format) the trailing comma should not be there.
Copy link
Collaborator

@petoalbert petoalbert left a comment

Choose a reason for hiding this comment

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

Thank you @erikvanoosten! I think this is a cleaner representation so I will merge this, but I am a bit confused if putting a comma at the end is really not supported. Can we discuss this first? Did you experience some issues with prometheus in an actual application?

The linked site contains this snippet, which in my interpretation allows an optional comma after the last label

metric_name [
   "{" label_name "=" `"` label_value `"` { "," label_name "=" `"` label_value `"` } [ "," ] "}"
] value [ timestamp ]

@erikvanoosten
Copy link
Contributor Author

Hi @petoalbert . I think you are right. I had not looked at the BNF expression too closely, I mostly looked at the examples. But indeed, on closer inspection there is indeed an optional comma at the end.

BTW, no, I didn't actually use this project yet (I will soon) so I did not experience any compatibility problems.

@petoalbert
Copy link
Collaborator

Okay, anyway I'll merge this because not having a trailing comma is cleaner in my opinion. I am happy to hear that you will use this project. 🙂

@petoalbert petoalbert merged commit 5396527 into zio:zio/series2.x Jan 10, 2024
12 checks passed
@erikvanoosten erikvanoosten deleted the fix-prometheus-format branch January 12, 2024 18:27
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