-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
OpenTelemetry Output Plugin #9109
OpenTelemetry Output Plugin #9109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla
!signed-cla |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤝 ✅ CLA has been signed. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like new artifacts were built from this PR. Get them here!
Artifact URLs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like new artifacts were built from this PR. Get them here!
Artifact URLs
Addressed the new comments, it looks like the failing test is caused by a race condition happening in a different plugin. |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
func (c *client) ping(ctx context.Context) error { | ||
// Loop until the context is canceled, allowing for retryable failures. | ||
for { | ||
err := c.connect(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, this is somewhat unexpected from my perspective. The task of this function is to test connectivity, so I'm not sure if it's a good idea to connect. However, if everybody else accepts this as is, I can live with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. Can you please say a few words regarding the possibility to use the v0.19 interface of the upstream client and thus get rid of most of your client implementation!?
Another comment is about connecting in ping
. To me it's unexpected, but this is not a blocker for me.
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Hey @srebhan Thanks for all the reviews! With regards to using the OTLP exporter instead of the current client code, in order to utilize it, it would require creating various implementing various components specified in the SDK (CheckpointSet, Aggregator) which are likely to change in the next iteration of the opentelemetry metrics signal specification. There's no exposed interface that allows me to use the protobuf more directly and pass in the slice of If you're ok w/ the current implementation, I would be happy to refactor the client code in the future, once the metrics API/SDK has stabilized. I just don't want to implement something that's not trivial (I spent some hours trying today, and i'm not sure the code is any simpler) and have to throw it away in the near future. I've updated the code to remove the separate ping function, and instead incorporated the check into the connect function. |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Looks like there's another PR implementing this as well #9228 |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
@srebhan not sure how to move this PR forward here, any help is much appreciated. |
@srebhan and some other Telegraf engineers will discuss the two OpenTelemetry output plugin PRs next week. We should have an answer for you @codeboten by Weds - I believe there's nothing we need from your right now. |
Thanks @sjwang90! |
@codeboten The team would like to proceed with #9228 and that PR is close to being merged. If you would like to take a review at the PR and address any concerns. Otherwise, thanks so much for your contribution. Either way we should be getting this OpenTelemetry Output plugin into master very soon! |
Thanks @sjwang90, I tested the other PR and couldn't see any data being output to the OpenTelemetry collector. See my comment here: Running the same test I did with the other PR with the code in this PR I see the following output:
|
@codeboten Thanks for your feedback. @jacobmarble is going to take a look at your comments. Let's provide any updates on #9228 to keep it all the discussion in the same place. |
Closing since #9228 has been merged. Thanks for help! |
Required for all PRs:
resolves #9108
This change takes the Stackdriver output plugin as a template and outputs the data in the OpenTelemetry protocol instead. It's currently state still needs: