[v2][adjuster] Implement adjuster for correct timestamps for clockskew #6392

mahadzaryab1 · 2024-12-22T16:26:10Z

Which problem is this PR solving?

Towards Implement adjusters to operate on OTLP data format #6344

Description of the changes

Implemented an adjuster to correct timestamps for clockskew.

How was this change tested?

Added unit tests

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: npm run lint and npm run test

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

codecov · 2024-12-22T16:33:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.23%. Comparing base (54ceda2) to head (8dba287).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #6392    +/-   ##
========================================
  Coverage   96.22%   96.23%            
========================================
  Files         363      364     +1     
  Lines       20748    20868   +120     
========================================
+ Hits        19965    20082   +117     
- Misses        599      601     +2     
- Partials      184      185     +1

Flag	Coverage Δ
badger_v1	`9.04% <ø> (ø)`
badger_v2	`1.64% <ø> (ø)`
cassandra-4.x-v1-manual	`15.04% <ø> (ø)`
cassandra-4.x-v2-auto	`1.58% <ø> (ø)`
cassandra-4.x-v2-manual	`1.58% <ø> (ø)`
cassandra-5.x-v1-manual	`15.04% <ø> (ø)`
cassandra-5.x-v2-auto	`1.58% <ø> (ø)`
cassandra-5.x-v2-manual	`1.58% <ø> (ø)`
elasticsearch-6.x-v1	`18.76% <ø> (+<0.01%)`	⬆️
elasticsearch-7.x-v1	`18.84% <ø> (+<0.01%)`	⬆️
elasticsearch-8.x-v1	`18.99% <ø> (ø)`
elasticsearch-8.x-v2	`1.64% <ø> (+<0.01%)`	⬆️
grpc_v1	`10.72% <ø> (+<0.01%)`	⬆️
grpc_v2	`7.98% <ø> (ø)`
kafka-v1	`9.40% <ø> (ø)`
kafka-v2	`1.64% <ø> (ø)`
memory_v2	`1.64% <ø> (ø)`
opensearch-1.x-v1	`18.88% <ø> (ø)`
opensearch-2.x-v1	`18.89% <ø> (+<0.01%)`	⬆️
opensearch-2.x-v2	`1.63% <ø> (-0.01%)`	⬇️
tailsampling-processor	`0.47% <ø> (ø)`
unittests	`95.08% <100.00%> (+0.01%)`	⬆️

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 · 2024-12-22T16:59:09Z

Let's make sure we reproduce the existing tests as close as possible

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

cmd/query/app/querysvc/adjuster/clockskew.go

yurishkuro · 2024-12-22T18:14:45Z

cmd/query/app/querysvc/adjuster/clockskew.go

+// hostKey derives a unique string representation of a host based on resource attributes.
+// This is used to determine if two spans are from the same host.
+func hostKey(resource ptrace.ResourceSpans) string {
+	if attr, ok := resource.Resource().Attributes().Get("ip"); ok {


We may need to reconsider this. Even in OpenTracing the tag "ip" was never standard, so why should we use it as a differentiator? In OTEL there are semantic conventions for host identity, we should be using those.

@yurishkuro I see these semantic conventions related to hosts: https://github.com/open-telemetry/opentelemetry-go/blob/main/semconv/v1.26.0/attribute_group.go#L3853-L3854. Do we want to replace this with HostIPKey?

I think we should try (in order):

host.id

host.ip

host.name

The host.ip is an array, I think it's ok to look at the first element only.

@yurishkuro Done. Let me know if this aligns with what you were thinking

cmd/query/app/querysvc/adjuster/clockskew.go

yurishkuro · 2024-12-22T18:17:55Z

cmd/query/app/querysvc/adjuster/clockskew.go

+
+const (
+	warningDuplicateSpanID       = "duplicate span IDs; skipping clock skew adjustment"
+	warningFormatInvalidParentID = "invalid parent span IDs=%s; skipping clock skew adjustment"


Why does this have format in the name? Are we warning about id format? I'd also tweak the error to be more specific - invalid is not really descriptive. I think here we mean that parent span id is not in the trace.

@yurishkuro I think the "format" is there is because this is a format string. I removed the word format and changed the warning string per your suggestion.

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

cmd/query/app/querysvc/adjuster/clockskew.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro · 2024-12-23T02:20:47Z

cmd/query/app/querysvc/adjuster/clockskew.go

+		return attr.Str()
+	}
+	if attr, ok := resource.Resource().Attributes().Get(string(otelsemconv.HostIPKey)); ok {
+		ips := attr.Slice()


I suspect some people may ignore setting a slice and set just a plain string, so I would handle that use case too.

cmd/query/app/querysvc/adjuster/clockskew.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

jaegertracing#6392) ## Which problem is this PR solving? - Towards jaegertracing#6344 ## Description of the changes - Implemented an adjuster to correct timestamps for clockskew. ## How was this change tested? - Added unit tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 added 2 commits December 22, 2024 11:14

Implement Clockskew Adjuster To Operate On OTLP Format

9b74d10

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Perform Minor Cleanups

687b20d

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 added the changelog:bugfix-or-minor-feature label Dec 22, 2024

mahadzaryab1 mentioned this pull request Dec 22, 2024

Implement adjusters to operate on OTLP data format #6344

Closed

13 tasks

Fix Variable Naming

cd906f4

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro reviewed Dec 22, 2024

View reviewed changes

mahadzaryab1 added 4 commits December 22, 2024 13:23

Write Unit Tests

a11a11a

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Add More Detail to godoc Comment

9a46aa7

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Invalid Parent Span ID Warning Message

aea9ef9

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Ignore gosec Linter Error

6d37f88

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 requested a review from yurishkuro December 22, 2024 21:15

mahadzaryab1 changed the title ~~[WIP][v2][adjuster] Implement adjuster for correct timestamps for clockskew~~ [v2][adjuster] Implement adjuster for correct timestamps for clockskew Dec 22, 2024

mahadzaryab1 marked this pull request as ready for review December 22, 2024 21:15

mahadzaryab1 requested a review from a team as a code owner December 22, 2024 21:15

dosubot bot added the v2 label Dec 22, 2024

yurishkuro reviewed Dec 22, 2024

View reviewed changes

cmd/query/app/querysvc/adjuster/clockskew.go Outdated Show resolved Hide resolved

mahadzaryab1 added 2 commits December 22, 2024 19:32

Add New Semantic Conventions

24b378f

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Address Feedback

a703698

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 requested a review from yurishkuro December 23, 2024 00:36

yurishkuro approved these changes Dec 23, 2024

View reviewed changes

mahadzaryab1 added 2 commits December 22, 2024 21:25

Address Feedback

344b7b6

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Handle Case Where Attribute Is String

8dba287

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 enabled auto-merge (squash) December 23, 2024 02:39

mahadzaryab1 merged commit e39609e into jaegertracing:main Dec 23, 2024
54 checks passed

mahadzaryab1 deleted the clockskew branch December 23, 2024 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2][adjuster] Implement adjuster for correct timestamps for clockskew #6392

[v2][adjuster] Implement adjuster for correct timestamps for clockskew #6392

mahadzaryab1 commented Dec 22, 2024 •

edited

Loading

codecov bot commented Dec 22, 2024 •

edited

Loading

yurishkuro commented Dec 22, 2024

yurishkuro Dec 22, 2024

mahadzaryab1 Dec 22, 2024

yurishkuro Dec 22, 2024

yurishkuro Dec 22, 2024

mahadzaryab1 Dec 23, 2024

yurishkuro Dec 22, 2024

mahadzaryab1 Dec 22, 2024

yurishkuro Dec 23, 2024

[v2][adjuster] Implement adjuster for correct timestamps for clockskew #6392

[v2][adjuster] Implement adjuster for correct timestamps for clockskew #6392

Conversation

mahadzaryab1 commented Dec 22, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Dec 22, 2024 • edited Loading

Codecov Report

yurishkuro commented Dec 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahadzaryab1 commented Dec 22, 2024 •

edited

Loading

codecov bot commented Dec 22, 2024 •

edited

Loading