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.

Sign up for GitHub

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

Jump to bottom

test: gcs object client test data race #12324

Merged
merged 3 commits into from
Mar 27, 2024
Merged

test: gcs object client test data race #12324

merged 3 commits into from
Mar 27, 2024

Conversation

paul1r
Copy link
Collaborator

@paul1r paul1r commented Mar 22, 2024

What this PR does / why we need it:

Before fix:

==================
WARNING: DATA RACE
Read at 0x00c00080b890 by goroutine 67:
  net/url.(*URL).Query()
      /opt/homebrew/Cellar/go/1.21.6/libexec/src/net/url/url.go:1115 +0x30
  google.golang.org/api/googleapi/transport.(*APIKey).RoundTrip()
      /Users/progers/dev/src/github.com/grafana/loki/vendor/google.golang.org/api/googleapi/transport/apikey.go:40 +0x108
  github.com/grafana/loki/pkg/storage/chunk/client/gcp.instrumentedTransport.RoundTrip()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/chunk/client/gcp/instrumentation.go:76 +0x68
  github.com/grafana/loki/pkg/storage/chunk/client/gcp.(*instrumentedTransport).RoundTrip()
      <autogenerated>:1 +0x64
  github.com/grafana/loki/pkg/storage/chunk/client/hedging.(*limitedHedgingRoundTripper).RoundTrip()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/chunk/client/hedging/hedging.go:144 +0x9c
  github.com/grafana/loki/pkg/storage/chunk/client/hedging.(*winnerTrackingRoundTripper).RoundTrip()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/chunk/client/hedging/hedging.go:157 +0x60
  github.com/cristalhq/hedgedhttp.(*hedgedTransport).RoundTrip.func2()
      /Users/progers/dev/src/github.com/grafana/loki/vendor/github.com/cristalhq/hedgedhttp/hedged.go:211 +0x94
  github.com/cristalhq/hedgedhttp.runInPool.func1()
      /Users/progers/dev/src/github.com/grafana/loki/vendor/github.com/cristalhq/hedgedhttp/hedged.go:305 +0x38

Previous write at 0x00c00080b890 by goroutine 64:
  google.golang.org/api/googleapi/transport.(*APIKey).RoundTrip()
      /Users/progers/dev/src/github.com/grafana/loki/vendor/google.golang.org/api/googleapi/transport/apikey.go:42 +0x164
  github.com/grafana/loki/pkg/storage/chunk/client/gcp.instrumentedTransport.RoundTrip()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/chunk/client/gcp/instrumentation.go:76 +0x68
  github.com/grafana/loki/pkg/storage/chunk/client/gcp.(*instrumentedTransport).RoundTrip()
      <autogenerated>:1 +0x64
  github.com/grafana/loki/pkg/storage/chunk/client/hedging.(*limitedHedgingRoundTripper).RoundTrip()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/chunk/client/hedging/hedging.go:144 +0x9c
  github.com/grafana/loki/pkg/storage/chunk/client/hedging.(*winnerTrackingRoundTripper).RoundTrip()
      /Users/progers/dev/src/github.com/grafana/loki/pkg/storage/chunk/client/hedging/hedging.go:157 +0x60
  github.com/cristalhq/hedgedhttp.(*hedgedTransport).RoundTrip.func2()
      /Users/progers/dev/src/github.com/grafana/loki/vendor/github.com/cristalhq/hedgedhttp/hedged.go:211 +0x94
  github.com/cristalhq/hedgedhttp.runInPool.func1()
      /Users/progers/dev/src/github.com/grafana/loki/vendor/github.com/cristalhq/hedgedhttp/hedged.go:305 +0x38

After fix:

go test . -race -count=20
ok  	github.com/grafana/loki/pkg/storage/chunk/client/gcp	28.149s

Which issue(s) this PR fixes:
Relates to: #8586

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@paul1r paul1r requested a review from a team as a code owner March 22, 2024 17:38
cyriltovena
cyriltovena approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@paul1r paul1r merged commit aca2a38 into main Mar 27, 2024
10 checks passed
@paul1r paul1r deleted the paul1r/gcs_object_client_test_data_race branch March 27, 2024 11:50
trevorwhitney added a commit that referenced this pull request Mar 27, 2024
@trevorwhitney
chore: merge with origin/main
99d57d2 
Squashed commit of the following:

commit 15dc2ba
Author: Christian Haudum <[email protected]>
Date:   Wed Mar 27 19:55:13 2024 +0100

    feat(bloomstore): Support for sharding blocks across multiple different directories (#12375)

    Signed-off-by: Christian Haudum <[email protected]>

commit c1edb82
Author: J Stickler <[email protected]>
Date:   Wed Mar 27 14:46:00 2024 -0400

    docs: fix formatting in structured metadata topic (#12382)

commit c9e5c7f
Author: Owen Diehl <[email protected]>
Date:   Wed Mar 27 10:27:44 2024 -0700

    chore(blooms): removes bloom-gw & bloom-compactor from all non-microservice targets (#12381)

commit 2b8db8b
Author: Travis Patterson <[email protected]>
Date:   Wed Mar 27 08:48:04 2024 -0600

    fix: attempt non-string label filtering when errors are present (#12378)

commit f99e6ac
Author: Dylan Guedes <[email protected]>
Date:   Wed Mar 27 10:51:21 2024 -0300

    docs: Add missing changelog entry for mTLS memcached addition (#12377)

commit 016daa5
Author: Karsten Jeschkies <[email protected]>
Date:   Wed Mar 27 14:28:32 2024 +0100

    fix: Track all OTLP metadata bytes in usage tracker. (#12376)

    Co-authored-by: Vladyslav Diachenko <[email protected]>

commit aca2a38
Author: Paul Rogers <[email protected]>
Date:   Wed Mar 27 07:50:25 2024 -0400

    test: gcs object client test data race (#12324)

commit bdad1d3
Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Date:   Wed Mar 27 10:52:30 2024 +0000

    chore(deps): update module google.golang.org/protobuf to v1.33.0 [security] (main) (#12137)

    Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
    Co-authored-by: Michel Hollands <[email protected]>

commit ca667aa
Author: Christian Haudum <[email protected]>
Date:   Wed Mar 27 09:53:33 2024 +0100

    chore(bloom-gw): Cleanup tracing implementation (#12368)

    The huge amount of `bloomgateway.ProcessTask` spans caused problem with Tempo's span limit, resulting in dropped/dangling spans.

    Since the processing time is now recorded also in a metrics.go-like stats log line, we can remove the spans and only add a log event with the summary to the main span of the request handler.

    This PR also unifies the event logging in the index gateway, and adds the real processing time (not aggregated processing time) to the request stats.

    ---
    Signed-off-by: Christian Haudum <[email protected]>

commit 120b76d
Author: Owen Diehl <[email protected]>
Date:   Wed Mar 27 00:16:07 2024 -0700

    chore(blooms): misc fixes (#12369)

    unbounds bloom-gw-client concurrency

    removes unused log_gateway_requests param

    adds note wrt bloom-gw results cache keygen issue

    add msg field to stats log

    sets bloomshipper download workers count to 16 default

commit 19c046f
Author: Christian Haudum <[email protected]>
Date:   Tue Mar 26 23:07:02 2024 +0100

    chore(blooms): Make max bloom page size for querying configurable (#12337)

    Signed-off-by: Christian Haudum <[email protected]>

commit 9810e8e
Author: Owen Diehl <[email protected]>
Date:   Tue Mar 26 11:57:27 2024 -0700

    chore(blooms): computed fields for bloomgw stats logging (#12367)

commit 6b3bbb3
Author: J Stickler <[email protected]>
Date:   Tue Mar 26 14:22:48 2024 -0400

    docs: Update release notes for 2.9.6 (#12365)

commit 30ac88b
Author: Owen Diehl <[email protected]>
Date:   Tue Mar 26 11:00:47 2024 -0700

    chore(blooms): increase blockpool by factor-of-2 (#12363)

commit d3266a1
Author: Pangidoan Butar <[email protected]>
Date:   Wed Mar 27 01:53:05 2024 +0800

    docs: Update structured_metadata.md (#12355)

    Co-authored-by: J Stickler <[email protected]>

commit c0c7a19
Author: J Stickler <[email protected]>
Date:   Tue Mar 26 11:30:00 2024 -0400

    docs: Update release notes for recent 2.9 releases (#12349)

commit 9af191f
Author: Christian Haudum <[email protected]>
Date:   Tue Mar 26 15:09:40 2024 +0100

    feat(bloom-gw): Add `metrics.go` style log line to bloom gateway `FilterChunks` call (#12354)

    This PR adds a "metrics.go" style log line to the bloom gateway that contains latencies for queueing, processing, post-processing, as well as number of chunks/series requested/filtered.

    This log line should give operators a better understanding where time is spent for individual requests.

    Example log line:

    ```
    ts=2024-03-26T13:16:11.869619964Z caller=spanlogger.go:109 component=bloom-gateway tenant=XXX method=bloomgateway.FilterChunkRefs user=XXX traceID=6239d49e1e88f88645bd7f1f6f1b85c8 level=info status=success tasks=1 series_requested=35 series_filtered=31 chunks_requested=103 chunks_filtered=93 queue_time=4.108128936s metas_fetch_time=22.040408ms blocks_fetch_time=1.284575ms processing_time=30.215863002s post_processing_time=16.084µs
    ```

    Signed-off-by: Christian Haudum <[email protected]>

commit 5c6a031
Author: Salva Corts <[email protected]>
Date:   Tue Mar 26 14:59:26 2024 +0100

    fix(blooms): Not filters should always match (#12358)

commit 64c7812
Author: Ashwanth <[email protected]>
Date:   Tue Mar 26 18:55:12 2024 +0530

    fix(codec): inject disable wrappers in ctx (#12352)

commit a36483b
Author: Owen Diehl <[email protected]>
Date:   Tue Mar 26 03:36:49 2024 -0700

    feat(blooms): bloom integration in query planning (#12208)

    Signed-off-by: Owen Diehl <[email protected]>

commit d5ecf9a
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue Mar 26 10:21:48 2024 +0000

    chore(deps): bump google.golang.org/protobuf from 1.32.0 to 1.33.0 in /operator (#12207)

    Signed-off-by: dependabot[bot] <[email protected]>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
    Co-authored-by: Periklis Tsirakidis <[email protected]>

commit 774b01d
Author: Ashwanth <[email protected]>
Date:   Tue Mar 26 13:15:54 2024 +0530

    fix(ruler): pass noop analyseRules to rules manager (#12353)

commit 03982c2
Author: arukiidou <[email protected]>
Date:   Tue Mar 26 16:10:31 2024 +0900

    docs: fix otlp guide - collector config example (#12328)

    Signed-off-by: junya koyama <[email protected]>
    Co-authored-by: Sandeep Sukhani <[email protected]>

commit 664e569
Author: Trevor Whitney <[email protected]>
Date:   Mon Mar 25 16:12:58 2024 -0600

    feat: prepare 3.0.0 release candidate (#12348)

    Release-As: 3.0.0-alpha.1

commit 40f695e
Author: Trevor Whitney <[email protected]>
Date:   Mon Mar 25 15:29:12 2024 -0600

    ci: prepare 3.0 release workflow (#12344)

    Release-As: 3.0.0-alpha.1

commit 04398a1
Author: Travis Patterson <[email protected]>
Date:   Mon Mar 25 14:52:24 2024 -0600

    fix: log state of 'disable pipeline wrappers' (#12345)
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
@paul1r @rhnasc
test: gcs object client test data race (grafana#12324)
3106c7c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cyriltovena cyriltovena cyriltovena approved these changes

Assignees
No one assigned
Labels
size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paul1r @cyriltovena