-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
pkg/config/legacy: add missing trace agent properties to legacy config converter. #2698
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2698 +/- ##
==========================================
+ Coverage 56.67% 57.26% +0.59%
==========================================
Files 380 380
Lines 24123 24238 +115
==========================================
+ Hits 13671 13881 +210
+ Misses 9537 9469 -68
+ Partials 915 888 -27
|
@olivielpeau can you PTAL if this is more along the lines of what you meant in #2678 (comment)? If yes, I will test this PR directly in trace and potentially merge it if afterwards if all is well with you too and the team. |
85c3f5e
to
c70dab8
Compare
c70dab8
to
a33c06c
Compare
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.
Left a few comments, overall LGTM 👍
I'm interested in how you'll be using this in the trace-agent, please ping me when those are ready :)
Thanks for looking. I'm going to try it out in trace agent and see if anything is missing. Will ping you on both PRs. |
@olivielpeau PTAL. Addressed your comments and submitted another minor fix. Also tried it out on trace agent. Will open PR soon once this gets merged (so I can vendor the commit inside trace-agent). |
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'll let you get approval from the Agent guild as well ;)
Had a cursory look at the trace-agent PR as well and it also looks good to me
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 👍
Adds missing trace agent properties to legacy config converter. As a follow-up to #2678