-
Notifications
You must be signed in to change notification settings - Fork 41
feat: add otlp exampler support #223
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
Conversation
@kirankumar-grootan Can you add a unit-test for this? |
@gaby I added a test case for this, which requires some setup for the OTLP part. I included a helper function within the same test file. Should I move it to a separate file, or is it better to keep it at the end of the test file? |
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.
Overall looks good to me, just two simple comments.
@kirankumar-grootan Added some comments, thanks for the PR 💪 |
@gaby addressed all the comments in the MR |
@kirankumar-grootan You may have to run "go mod tidy" |
applied this is the version im using in my local |
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.
LGTM, I do wonder if making the "EnableOpenMetrics" a param makes sense though
@kirankumar-grootan Thoughts? |
@kirankumar-grootan can you run "go mod tidy" once more. Thanks. Will merge after |
Hmm, I hadn't considered making it optional since we're using OTLP and metrics for all our services. The fact is, traces are only scraped with exemplars when the I will check and let you know if there is a better way to turn this on when required. |
no changes to commit after |
That's fine then. |
Ahhh, you have to rebase your branch. Its complaining about merge conflict with |
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.
LGTM
@kirankumar-grootan For some reason GitHub says that commits do not have verified signatures, so it doesn't let me merge it. |
7140b96
to
88a78f9
Compare
db2313c
to
24813f5
Compare
IDK what went wrong with my gpg key. now all commits are verified |
4fbaddb
to
6fbc534
Compare
6fbc534
to
315441a
Compare
315441a
to
13fae73
Compare
@kirankumar-grootan That fixed it. Will merge today. Thank you |
Fixes #207