-
Notifications
You must be signed in to change notification settings - Fork 211
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
*: Allow using persistence with WAL and object storage #1403
Conversation
ad2a24f
to
406a061
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.
It's great to see how we're building and passing the options into FrostDB! 💯
@@ -140,13 +148,38 @@ func Run(ctx context.Context, logger log.Logger, reg *prometheus.Registry, flags | |||
return runScraper(ctx, logger, reg, tracerProvider, flags, version, cfg) | |||
} | |||
|
|||
bucketCfg, err := yaml.Marshal(cfg.ObjectStorage.Bucket) | |||
if err != nil { | |||
level.Error(logger).Log("msg", "failed to marshal object storage bucket config", "err", err) |
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.
Aren't we logging the same error twice if we also return it?
Lines 57 to 61 in e48920c
err := parca.Run(ctx, logger, registry, flags, version) | |
if err != nil { | |
level.Error(logger).Log("msg", "Program exited with error", "err", err) | |
os.Exit(1) | |
} |
I can see this was already a thing with the error handling in this function before so I'm fine with having these in. Maybe creating an issue to clean it up would be good?
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.
Agreed! 💯
Since parca-dev/parca#1403 the syntax here doesn't work, so align with the new parca.yaml from the main repo
Since parca-dev/parca#1403 the syntax here doesn't work, so align with the new parca.yaml from the main repo
This plugs all the persistence features of FrostDB into Parca.