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

[v2][query] Apply "Max Clock Skew Adjust" setting #6566

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

dnaka91
Copy link
Contributor

@dnaka91 dnaka91 commented Jan 18, 2025

Apply the max_clock_skew_adjust setting to the query service during initialization.

The maximum clock skew adjustment setting for the query component is properly read during initialization, but not applied in the v2 server in the end. This causes it to always default to 1 second, regardless of the configuration value.

Which problem is this PR solving?

Description of the changes

  • Initialize and set the Adjuster on both the v1 and v2 query options.
  • The config defaults to 0 so applying this unconditionally would now disable the clock skew adjustment instead of defaulting to 1s? However, the same seems to be done in the older services from v1.
    • Having a default of 1s instead of 0 (which disables adjustments completely) might have been an accident?

How was this change tested?

  • Not testing so far.

Checklist

cmd/jaeger/config.yaml Outdated Show resolved Hide resolved
@dnaka91 dnaka91 marked this pull request as ready for review January 22, 2025 21:50
@dnaka91 dnaka91 requested a review from a team as a code owner January 22, 2025 21:50
@dnaka91 dnaka91 requested a review from jkowall January 22, 2025 21:50
@dosubot dosubot bot added the v2 label Jan 22, 2025
@dnaka91
Copy link
Contributor Author

dnaka91 commented Jan 22, 2025

@yurishkuro I adjusted the PR to instead pass down the value through the service options and make the adjuster internal. I hope I understood your comment correctly by that change idea.

There didn't seem to be any reason to keep the adjuster public and imho this way it's clearer as there is only one place that sets the adjuster field.

@yurishkuro
Copy link
Member

We have a bit of a mess right now:

  • cmd/query/app/flags.go uses CLI flag to set qOpts.MaxClockSkewAdjust and then uses that to initialize Adjuster in v1/v2 QueryServiceOptions
  • at the same time, v1/v2 QueryService constructors check if Adjuster is null and initialize it with defaultMaxClockSkewAdjust
  • meanwhile cmd/jaeger/internal/extension/jaegerquery/server.go creates v1/v2 QueryServiceOptions without Adjuster

As a result, in v1 the user-settings are respected, but in v2 we always end up with defaultMaxClockSkewAdjust, even though the config has a setting.

I would like to minimize duplication. My suggestion is:

  • remove Adjuster from the options altogether,
  • make it a private field in the QS,
  • initialize it in the constructor,
  • extend Options structs to contain MaxClockSkewAdjust field
  • initialize this field in cmd/query/app/flags.go and cmd/jaeger/internal/extension/jaegerquery/server.go

cc @mahadzaryab1

cmd/query/app/flags.go Outdated Show resolved Hide resolved
@dnaka91 dnaka91 force-pushed the push-swuwnoukltqx branch 2 times, most recently from 5867d9c to 43daedc Compare January 23, 2025 00:54
cmd/jaeger/config.yaml Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.93%. Comparing base (10bacb7) to head (e208918).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6566      +/-   ##
==========================================
- Coverage   95.94%   95.93%   -0.02%     
==========================================
  Files         365      365              
  Lines       20616    20618       +2     
==========================================
- Hits        19781    19780       -1     
- Misses        636      638       +2     
- Partials      199      200       +1     
Flag Coverage Δ
badger_v1 9.92% <ø> (ø)
badger_v2 1.84% <ø> (ø)
cassandra-4.x-v1-manual 14.93% <ø> (ø)
cassandra-4.x-v2-auto 1.83% <ø> (ø)
cassandra-4.x-v2-manual 1.83% <ø> (ø)
cassandra-5.x-v1-manual 14.93% <ø> (ø)
cassandra-5.x-v2-auto 1.83% <ø> (ø)
cassandra-5.x-v2-manual 1.83% <ø> (ø)
elasticsearch-6.x-v1 19.30% <ø> (ø)
elasticsearch-7.x-v1 19.38% <ø> (ø)
elasticsearch-8.x-v1 19.56% <ø> (ø)
elasticsearch-8.x-v2 1.84% <ø> (ø)
grpc_v1 10.90% <ø> (ø)
grpc_v2 7.89% <ø> (ø)
kafka-3.x-v1 10.21% <ø> (ø)
kafka-3.x-v2 1.84% <ø> (ø)
memory_v2 1.84% <ø> (ø)
opensearch-1.x-v1 19.44% <ø> (ø)
opensearch-2.x-v1 19.44% <ø> (ø)
opensearch-2.x-v2 1.84% <ø> (ø)
tailsampling-processor 0.48% <ø> (ø)
unittests 94.82% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yurishkuro
Copy link
Member

SIGSEGV in unit tests, please make sure make test is successful.

@dnaka91 dnaka91 force-pushed the push-swuwnoukltqx branch 2 times, most recently from 7efa229 to 61b14c9 Compare January 23, 2025 07:38
@dnaka91
Copy link
Contributor Author

dnaka91 commented Jan 23, 2025

Sorry for that, didn't test the latest few changes anymore before heading to bed. Tests should be passing again now 🙇‍♂️

@dnaka91
Copy link
Contributor Author

dnaka91 commented Jan 29, 2025

Ah I see some merge conflicts have come up, let me fix those (hopefully) real quick.

Apply the `max_clock_skew_adjust` setting to the query service during
initialization.

The maximum clock skew adjustment setting for the query component is
properly read during initialization, but not applied in the v2 server in
the end. This causes it to always default to 1 second, regardless of the
configuration value.

Signed-off-by: Dominik Nakamura <[email protected]>
@yurishkuro yurishkuro merged commit fa74006 into jaegertracing:main Jan 30, 2025
55 checks passed
@yurishkuro
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Max clock skew setting not applied in v2
3 participants