-
Notifications
You must be signed in to change notification settings - Fork 86
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
feat: allow updating export options at runtime #974
Conversation
fc2baae
to
f8da410
Compare
17fc0fd
to
a54ec88
Compare
a54ec88
to
58f5e6b
Compare
ffd0c0c
to
c09542e
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.
Nice!
Partial review - will finish up later this week.
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.
Apologies for the delay, finally finished review.
Looks like this is a combination change that depends on #1001. Is there any way this can be separated out? It's difficult to review the combined changes at once.
It already is -- go to the "Files changed" tab and on the top right directly underneath the tabs, there's a drop-down that says "All commits" where you can select which commit to review. Or do you mean you want this change without being rebased on top of the other change? |
Ah ok - it wasn't clear to me that I should just review that commit. If that's the case, I think most of my review comments outside of the rule-evaluator ones still apply - thanks for clarifying :) |
1730089
to
7da8e19
Compare
e46c512
to
e69c5c3
Compare
7da8e19
to
946a794
Compare
946a794
to
7f737b2
Compare
7f737b2
to
6f0331a
Compare
0978dc1
to
42954eb
Compare
42954eb
to
d093a09
Compare
d093a09
to
7d27ce9
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.
@pintohutch thanks! I made some changes you requested. Please take another look!
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.
Awesome - I think it mostly LGTM, though with a change of this scope I'd love some co-review love from @bwplotka if possible.
A few comments and questions. Once those are resolved, I think it's good to
63a60d8
to
0755bbf
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.
LGTM!
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.
Nice! Generally LGTM, it's nice as it enables collector improvement of no restart for exporter config / label setting.
I would focus on killing testing code in prod code though, it's getting bit out of control, but I will understand if @TheSpiritXIII don't have time for that here. Plus I would NOT modify options (or config) in apply method, feels it can cause surprised going forward.
0755bbf
to
0203c43
Compare
0203c43
to
758aa5a
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.
Thanks for great work on this - it wasn't easy. What I like about this, is that it actually is more consistent with Prometheus OSS, PRW handling is hot-configurable 👍🏽
Thanks for addressing all tricky comments and reducing testing code in prod code 💪🏽
Great effort on tests too!
_, err := google.FindDefaultCredentials(ctx) | ||
if err != nil { |
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.
_, err := google.FindDefaultCredentials(ctx) | |
if err != nil { | |
if _, err := google.FindDefaultCredentials(ctx); err != nil { |
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.
Small nit so merging PR without this
@@ -74,7 +74,9 @@ func (l *localExportWithGCM) Ref() string { return "export-pkg-with-gcm" } | |||
func (l *localExportWithGCM) start(t testing.TB, _ e2e.Environment) (v1.API, map[string]string) { | |||
t.Helper() | |||
|
|||
ctx := context.Background() | |||
ctx, cancel := context.WithCancel(signals.SetupSignalHandler()) |
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.
woooo didn't know this, so good
Pre-req: #1001
ApplyConfig
now takesExporterOpts
, which allows runtime updates.exporter.New
now takes a context andRun
no longer does, which means theExporter
uses the same context throughout its lifetime.These changes allow us to update the metric client at runtime. When a metric client is updated, it may take: