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

fluent-bit: edge case tests #1096

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

JensErat
Copy link
Contributor

@JensErat JensErat commented Oct 1, 2019

What this PR does / why we need it:

This PR adds some edge case tests to the fluent-bit Loki shipping plugin I added in my draft of the nested JSON relabeling code. All of them run successfully without any code fixes, but they somewhat define the expected behavior for non-string contents. fluent-bit can parse JSON and put it into labels, so one must expect arbitrary JSON (and thus nested maps) with integers, arrays and so on.

If the current behavior (like list {42, 43} passed as [42 43] to Loki) are not desired, what should they look like? Does Loki support numeric labels? Currently, the plugin just converts them to strings.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@CLAassistant
Copy link

CLAassistant commented Oct 1, 2019

CLA assistant check
All committers have signed the CLA.

@cyriltovena
Copy link
Contributor

Does that actually work the way I have setup the config for you ? I remember you took a slightly different approach ?

@JensErat JensErat mentioned this pull request Oct 1, 2019
2 tasks
@JensErat
Copy link
Contributor Author

JensErat commented Oct 1, 2019

Does that actually work the way I have setup the config for you ? I remember you took a slightly different approach ?

I was just replying in the merged PR. "Compatible" is to be defined -- it fits our use case (and actually I prefer your solution with separate configuration for relabeling and dropping labels to the unified approach I had: configuration is easier to read and code seems more simple). Thank you for the good job!

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@cyriltovena cyriltovena merged commit c4913bf into grafana:master Oct 1, 2019
@JensErat JensErat deleted the jenserat/edgecase-tests branch October 1, 2019 16:44
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.

None yet

3 participants