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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dnaka91
Copy link

@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

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]>
@@ -100,6 +102,10 @@ func (s *server) Start(ctx context.Context, host component.Host) error {
if err := s.addArchiveStorage(&opts, &v2opts, host); err != nil {
return err
}

opts.Adjuster = adjuster.Sequence(querysvc.StandardAdjusters(s.config.MaxClockSkewAdjust)...)
v2opts.Adjuster = v2adjuster.Sequence(v2adjuster.StandardAdjusters(s.config.MaxClockSkewAdjust)...)
Copy link
Member

Choose a reason for hiding this comment

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

@mahadzaryab1 did we miss adjusters altogether in query extension?

@@ -32,6 +32,7 @@ extensions:
traces_archive: another_store
ui:
config_file: ./cmd/jaeger/config-ui.json
max_clock_skew_adjust: 5s
Copy link
Member

Choose a reason for hiding this comment

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

we don't want this in the default config

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

Successfully merging this pull request may close these issues.

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