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

Add slogAdapter #6405

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Add slogAdapter #6405

merged 2 commits into from
Dec 9, 2024

Conversation

SungJin1212
Copy link
Contributor

This PR adds a slogAdapter converting go-kit/log to slog.

It would be good to use slogAdapter to upgrade the Prometheus version. We could implement features included in Prometheus v3.0.0, like OTLP metadata, OOO ingestion native histogram, UTF-8 support, and so on.

After that, we could migrate whole logs to slog if needed.

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

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@dosubot dosubot bot added go Pull requests that update Go code sync-vendor labels Dec 6, 2024
@yeya24
Copy link
Contributor

yeya24 commented Dec 6, 2024

This doesn't seem relevant.

==================
WARNING: DATA RACE
Write at 0x0000037316f7 by goroutine 39:
  github.com/cortexproject/cortex/pkg/querier/tripperware.NewQueryTripperware()
      /__w/cortex/cortex/pkg/querier/tripperware/roundtrip.go:112 +0x7a
  github.com/cortexproject/cortex/pkg/querier/tripperware/queryrange.TestRoundTrip()
      /__w/cortex/cortex/pkg/querier/tripperware/queryrange/query_range_middlewares_test.go:71 +0x42b
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1743 +0x44

Previous read at 0x0000037316f7 by goroutine 72:
  github.com/prometheus/prometheus/promql/parser.(*parser).newAggregateExpr()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/promql/parser/parse.go:450 +0x1aa
  github.com/prometheus/prometheus/promql/parser.(*yyParserImpl).Parse()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/promql/parser/generated_parser.y.go:1072 +0x1568
  github.com/prometheus/prometheus/promql/parser.(*parser).parseGenerated()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/promql/parser/parse.go:895 +0xc4
  github.com/prometheus/prometheus/promql/parser.(*parser).ParseExpr()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/promql/parser/parse.go:100 +0xcf
  github.com/prometheus/prometheus/promql/parser.ParseExpr()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/promql/parser/parse.go:171 +0xda
  github.com/cortexproject/cortex/pkg/querier/tripperware/queryrange.Test_evaluateAtModifier.func1()
      /__w/cortex/cortex/pkg/querier/tripperware/queryrange/split_by_interval_test.go:398 +0x1f9
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1743 +0x44

Goroutine 39 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1743 +0x825
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:2168 +0x85
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1690 +0x226
  testing.runTests()
      /usr/local/go/src/testing/testing.go:2166 +0x8be
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:2034 +0xf17
  main.main()
      _testmain.go:103 +0x164

Goroutine 72 (finished) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1743 +0x825
  github.com/cortexproject/cortex/pkg/querier/tripperware/queryrange.Test_evaluateAtModifier()
      /__w/cortex/cortex/pkg/querier/tripperware/queryrange/split_by_interval_test.go:388 +0x233
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1743 +0x44
==================
--- FAIL: TestSplitQuery (0.00s)
    --- FAIL: TestSplitQuery/1 (0.01s)
        testing.go:[139](https://github.com/cortexproject/cortex/actions/runs/12193243541/job/34015175708?pr=6405#step:6:140)9: race detected during execution of test
--- FAIL: TestConstSplitter_generateCacheKey (0.00s)
    testing.go:1399: race detected during execution of test
--- FAIL: TestRoundTrip (0.06s)
    testing.go:1399: race detected during execution of test
--- FAIL: TestMergeAPIResponses (0.00s)
    testing.go:1399: race detected during execution of test
--- FAIL: TestResponseWithStats (0.00s)
    testing.go:1399: race detected during execution of test
--- FAIL: TestShouldCache (0.00s)
    --- FAIL: TestShouldCache/negative_offset_on_subqueries (0.00s)
        testing.go:1399: race detected during execution of test
--- FAIL: TestResponse (0.00s)
    testing.go:1399: race detected during execution of test
--- FAIL: TestRequest (0.00s)
    testing.go:1399: race detected during execution of test
--- FAIL: TestSplitByDay (0.03s)
    testing.go:1399: race detected during execution of test
--- FAIL: TestHandleHit (0.02s)
    testing.go:1399: race detected during execution of test
--- FAIL: TestNextIntervalBoundary (0.03s)
    testing.go:1399: race detected during execution of test
--- FAIL: TestLimitsMiddleware_MaxQueryLookback (0.00s)
    testing.go:1399: race detected during execution of test
--- FAIL: TestCompressedResponse (0.00s)
    testing.go:1399: race detected during execution of test
--- FAIL: TestResultsCacheMaxFreshness (0.00s)
    --- FAIL: TestResultsCacheMaxFreshness/0 (0.25s)
        testing.go:1399: race detected during execution of test
FAIL
FAIL	github.com/cortexproject/cortex/pkg/querier/tripperware/queryrange	0.442s

@SungJin1212
Copy link
Contributor Author

SungJin1212 commented Dec 6, 2024

@yeya24
It seems relevant to #6355.
It seems caused by concurrent access to EnableExperimentalFunctions.

CHANGELOG.md Outdated Show resolved Hide resolved
@SungJin1212 SungJin1212 force-pushed the Add-slogAdapter branch 5 times, most recently from dea89dc to be2fd7a Compare December 6, 2024 08:38

// GoKitLogToSlog convert go-kit/log to slog
// usage: logutil.GoKitLogToSlog(gokitLogger)
func GoKitLogToSlog(logger log.Logger) *slog.Logger {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should name this function to PrometheusLoggerToSlot and have another function for GoKitLogToSlog which takes an additional level parameter?

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 9, 2024
@yeya24 yeya24 merged commit 7e444e7 into cortexproject:master Dec 9, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code lgtm This PR has been approved by a maintainer size/M sync-vendor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a slogAdapter converting go-kit/log to slog
3 participants