Skip to content
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

Configuration documentation improvements #2318

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

adityacs
Copy link
Contributor

@adityacs adityacs commented Jul 8, 2020

What this PR does / why we need it:
This is an attempt to improve the configuration doc. This PR also updates cortex to master

@@ -67,6 +68,11 @@ type EngineOpts struct {
MaxLookBackPeriod time.Duration `yaml:"max_look_back_period"`
}

func(opts *EngineOpts) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
f.DurationVar(&opts.Timeout, prefix+".engine.timeout", 3*time.Minute, "Timeout for query execution")
f.DurationVar(&opts.MaxLookBackPeriod, prefix+".engine.max-lookback-period", 30*time.Second, "MaxLookBackPeriod is the maximum amount of time to look back for log lines. Used only for instant log queries")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default is probably too short for a default, what is the behavior now, do we just support infinite lookback if you don't define this?

Copy link
Contributor

@cyriltovena cyriltovena Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only usde for instant log queries. It was already the default but we could change it to 5m if you want.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5m seems more reasonable to me as a default I think 👍

@slim-bean
Copy link
Collaborator

Initially I was hesitant to make the breaking changes to the command line configs, but these are pretty obscure configs and I think this is fine 👍 always better to do this stuff sooner than later.

@adityacs adityacs force-pushed the doc_improvements branch 2 times, most recently from 4fc197f to 9a2ab80 Compare July 10, 2020 02:42
@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2020

Codecov Report

Merging #2318 into master will decrease coverage by 0.03%.
The diff coverage is 22.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2318      +/-   ##
==========================================
- Coverage   61.50%   61.47%   -0.04%     
==========================================
  Files         160      160              
  Lines       13422    13426       +4     
==========================================
- Hits         8255     8253       -2     
- Misses       4546     4551       +5     
- Partials      621      622       +1     
Impacted Files Coverage Δ
pkg/querier/querier.go 63.08% <0.00%> (-0.19%) ⬇️
pkg/storage/store.go 62.50% <0.00%> (ø)
pkg/logql/engine.go 88.67% <25.00%> (-1.71%) ⬇️
pkg/ingester/ingester.go 51.20% <100.00%> (ø)
pkg/logql/evaluator.go 92.46% <0.00%> (-0.41%) ⬇️

@slim-bean
Copy link
Collaborator

I think the only thing I'd like to see added to this is an entry in the operations/upgrade guide explaining the config changes!

@owen-d
Copy link
Member

owen-d commented Jul 13, 2020

I think this looks great, but please avoid force pushing if not necessary. It's difficult to see incremental changes introduced by successive commits/edits.

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR, thanks.

@adityacs
Copy link
Contributor Author

@owen-d

but please avoid force pushing if not necessary.

I did some blunder while rebasing, that's the reason you see many force pushes here. Sorry for that

@owen-d owen-d merged commit 0885a49 into grafana:master Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants