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

Add prometheus remote write serializer #8360

Merged
merged 2 commits into from
Dec 16, 2020

Conversation

aarnaud
Copy link
Contributor

@aarnaud aarnaud commented Nov 3, 2020

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Related to #8346
Depends if we prefer used a dedicated serializer or not

@aarnaud
Copy link
Contributor Author

aarnaud commented Nov 3, 2020

Hi @ssoroka I create a new PR for explore remote write serializer if we prefer or not a dedicated serializer.

This serializer use some functions from Prometheus serializer when it's possible. sometime it's the same fonction copied with type changed

@aarnaud aarnaud force-pushed the feat/remote-write-serializer branch 2 times, most recently from f7cfccb to d287e96 Compare November 3, 2020 23:01
@aarnaud aarnaud changed the title WIP add prometheus remote write serializer Add prometheus remote write serializer Nov 4, 2020
@aarnaud
Copy link
Contributor Author

aarnaud commented Nov 6, 2020

@ssoroka I think dedicated serializer is less intrusive and more maintainable

@ssoroka
Copy link
Contributor

ssoroka commented Nov 25, 2020

Thanks for this. Will review it shortly.

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

Looks pretty complete, just seems to be missing a README, which should show example usage and mention that it's supposed to be used with the http output.

@aarnaud aarnaud force-pushed the feat/remote-write-serializer branch from 0568171 to 2be0757 Compare December 2, 2020 14:41
@jamal
Copy link

jamal commented Dec 16, 2020

Hi, I was curious if there were plans to get this merged in. Thanks!

@aarnaud
Copy link
Contributor Author

aarnaud commented Dec 16, 2020

who can merge because this PR is approved ?

@ssoroka ssoroka merged commit 045c3c1 into influxdata:master Dec 16, 2020
@aarnaud
Copy link
Contributor Author

aarnaud commented Dec 16, 2020

thanks @ssorathia ;-)

ssoroka pushed a commit that referenced this pull request Dec 16, 2020
tsharju pushed a commit to aiven/telegraf that referenced this pull request Jan 8, 2021
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
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.

4 participants