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

Update loki to cortex master #2030

Merged
merged 1 commit into from
May 13, 2020
Merged

Conversation

adityacs
Copy link
Collaborator

@adityacs adityacs commented May 1, 2020

What this PR does / why we need it:
Update loki to cortex master

Which issue(s) this PR fixes:
Fixes #1786

@adityacs
Copy link
Collaborator Author

adityacs commented May 1, 2020

Checking on failing tests

@adityacs adityacs force-pushed the cortex_master branch 2 times, most recently from d251cfc to 2f36328 Compare May 3, 2020 11:01
@codecov-io
Copy link

codecov-io commented May 3, 2020

Codecov Report

Merging #2030 into master will decrease coverage by 0.02%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2030      +/-   ##
==========================================
- Coverage   63.90%   63.87%   -0.03%     
==========================================
  Files         133      133              
  Lines       10217    10223       +6     
==========================================
+ Hits         6529     6530       +1     
- Misses       3202     3205       +3     
- Partials      486      488       +2     
Impacted Files Coverage Δ
pkg/loki/modules.go 15.78% <0.00%> (ø)
pkg/querier/querier.go 71.19% <75.00%> (-0.24%) ⬇️
pkg/querier/queryrange/prometheus.go 90.24% <100.00%> (+1.05%) ⬆️
pkg/querier/queryrange/roundtrip.go 79.36% <100.00%> (+0.16%) ⬆️
pkg/storage/store.go 70.27% <100.00%> (ø)
pkg/promtail/targets/tailer.go 76.13% <0.00%> (-2.28%) ⬇️
pkg/logql/evaluator.go 90.69% <0.00%> (-0.59%) ⬇️

@@ -43,6 +43,7 @@ type Config struct {
ExtraQueryDelay time.Duration `yaml:"extra_query_delay,omitempty"`
IngesterMaxQueryLookback time.Duration `yaml:"query_ingesters_within,omitempty"`
Engine logql.EngineOpts `yaml:"engine,omitempty"`
MaxConcurrent int `yaml:"max_concurrent"`
Copy link
Collaborator Author

@adityacs adityacs May 3, 2020

Choose a reason for hiding this comment

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

this is the only configuration which is used in updated cortex code of frontend worker. I have modified the code accordingly here https://github.com/grafana/loki/blob/2f3632878fbf31e31f2d2a998288d1eb675cb479/pkg/loki/modules.go#L160

Hope this change is fine

@@ -197,6 +199,7 @@ func NewMetricTripperware(
)

var c cache.Cache
tombstone := purger.NewTombstonesLoader(nil, nil)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have the purger config in Loki yet. Should we bring this config from cortex into this master update?

Still, we can initialize purger with nil DeleteStore. Initializing purger with nil DeleteStore should be noop.

@sandeepsukhani Hope I am correct here.

@adityacs adityacs force-pushed the cortex_master branch 2 times, most recently from 7ff859a to d77e614 Compare May 3, 2020 12:45
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

I think overall it looks good. Just some minor nits.

@@ -205,6 +207,7 @@ func NewMetricTripperware(
limits,
codec,
extractor,
tombstone,
Copy link
Contributor

Choose a reason for hiding this comment

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

@adityacs you can pass nil here instead of creating NewTombstonesLoader above, ResultsCacheMiddleware should take care of it.

@@ -50,7 +51,8 @@ type store struct {

// NewStore creates a new Loki Store using configuration supplied.
func NewStore(cfg Config, storeCfg chunk.StoreConfig, schemaCfg chunk.SchemaConfig, limits storage.StoreLimits, registerer prometheus.Registerer) (Store, error) {
s, err := storage.NewStore(cfg.Config, storeCfg, schemaCfg, limits, registerer)
tombstone := purger.NewTombstonesLoader(nil, nil)
s, err := storage.NewStore(cfg.Config, storeCfg, schemaCfg, limits, registerer, tombstone)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can pass nil here as well for tombstones loader to NewStore.

@adityacs
Copy link
Collaborator Author

adityacs commented May 5, 2020

@sandeepsukhani have made the requested changes.

@cyriltovena
Copy link
Contributor

Please @sandeepsukhani can you ensure follow up of this PR ? Thank you !

Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

LGTM

@sandeepsukhani sandeepsukhani merged commit 199e334 into grafana:master May 13, 2020
cyriltovena pushed a commit to cyriltovena/loki that referenced this pull request Jun 11, 2021
All code paths return nil as the error, so we can simplify the code.

Signed-off-by: Bryan Boreham <bryan@weave.works>
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.

Tuned down log from table manager when running boltdb
5 participants