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

Fix regression when parsing numbers in Push request #10550

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

salvacorts
Copy link
Contributor

What this PR does / why we need it:
Even though the Loki HTTP API docs for the push endpoint state that the stream label values should be strings, we previously didn't enforce this requirement. With #9694, we started enforcing this requirement, and that broke some users.

In this PR we are reverting this type of assertion and adding a bunch of tests to avoid the regression in the future.

Copy link
Contributor

@vlad-diachenko vlad-diachenko left a comment

Choose a reason for hiding this comment

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

Looks brilliant 💎 thanks @salvacorts

@salvacorts salvacorts marked this pull request as ready for review September 12, 2023 07:48
@salvacorts salvacorts requested a review from a team as a code owner September 12, 2023 07:48
@salvacorts salvacorts merged commit 5ab9515 into main Sep 12, 2023
6 checks passed
@salvacorts salvacorts deleted the salvacorts/fix-push-payload-string-label-value branch September 12, 2023 07:49
@salvacorts salvacorts added type/bug Somehing is not working as expected backport k167 labels Sep 12, 2023
grafanabot pushed a commit that referenced this pull request Sep 12, 2023
**What this PR does / why we need it**:
Even though the[ Loki HTTP API docs for the push endpoint][1] state that
the stream label values should be strings, we previously didn't enforce
this requirement. With #9694, we
started enforcing this requirement, and that broke some users.

In this PR we are reverting this type of assertion and adding a bunch of
tests to avoid the regression in the future.

[1]:
https://grafana.com/docs/loki/latest/reference/api/#push-log-entries-to-loki

(cherry picked from commit 5ab9515)
salvacorts added a commit that referenced this pull request Sep 12, 2023
Backport 5ab9515 from #10550

---

**What this PR does / why we need it**:
Even though the[ Loki HTTP API docs for the push endpoint][1] state that
the stream label values should be strings, we previously didn't enforce
this requirement. With #9694, we
started enforcing this requirement, and that broke some users.

In this PR we are reverting this type of assertion and adding a bunch of
tests to avoid the regression in the future.


[1]:
https://grafana.com/docs/loki/latest/reference/api/#push-log-entries-to-loki

Co-authored-by: Salva Corts <salva.corts@grafana.com>
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
**What this PR does / why we need it**:
Even though the[ Loki HTTP API docs for the push endpoint][1] state that
the stream label values should be strings, we previously didn't enforce
this requirement. With grafana#9694, we
started enforcing this requirement, and that broke some users.

In this PR we are reverting this type of assertion and adding a bunch of
tests to avoid the regression in the future.


[1]:
https://grafana.com/docs/loki/latest/reference/api/#push-log-entries-to-loki
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport k167 size/L type/bug Somehing is not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants