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

Adjuster fixes. #606

Closed
wants to merge 5 commits into from
Closed

Adjuster fixes. #606

wants to merge 5 commits into from

Conversation

asp24
Copy link

@asp24 asp24 commented Dec 21, 2017

For some app, it is hard to determine IP tag value. That's why for some cases clock-skew adjustment done wrong. In the PR multiple tags support was added.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3157f56 on code-tool:adjuster-fix into 40ca25b on jaegertracing:master.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

+1

@@ -53,6 +51,10 @@ const (
warningFormatInvalidParentID = "invalid parent span IDs=%s; skipping clock skew adjustment"
)

var (
hostKeyTags = []string{"jaeger.hostname", "ip"}
Copy link
Member

Choose a reason for hiding this comment

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

also should add "hostname", it's used in Go client

{
tags: []model.KeyValue{
model.String("ip", "1.2.3.4"),
model.String("jaeger.hostname", "localhost"),
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't ip be given priority in this case?

Now we try to find the tag from the predefined set which is most common in the process tags
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f1c0915 on code-tool:adjuster-fix into 40ca25b on jaegertracing:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 1ad0a73 on code-tool:adjuster-fix into 40ca25b on jaegertracing:master.

@yurishkuro
Copy link
Member

I don't think looking for a common host tag is a good idea. Different services may be instrumented with different versions of Jaeger clients, in different languages. Ideally we'd want them all to converge on the same way of representing the host identity, but even if we did this in the latest versions of all clients it will take a long time for all services in a large org to upgrade.

The host identity is used to ensure that the timestamp adjustments from spans from the same host are the same. While this may look like it might apply to different services, in practice it's more likely that when service A calls service B the instances of those two services will be running on different hosts. So the problem is really avoiding ts adjustments from spans in the same service, which is a problem that does not require a common host tag because those spans will all come from the same version of the client lib, therefore any stable algorithm of selecting a host tag will do, such as what you had in the first commit by simply iterating through an array of known host tags (the order in the array implies a priority).

@yurishkuro
Copy link
Member

Per offline discussion, my proposal is:

  • we could build a smarter comparison, e.g.have a struct Address { ip, hostname } and a function
    IsSame(a1, a2) { return a1.ip == a2.ip || a1.hostname == a2.hostname }
  • it still misses the equivalence case when ipv4 and ipv6 map to the same address, but it could also be taken into account, e.g. if v4 := ipv6.IPv4(); v4 != nil { return v4 }

@yurishkuro
Copy link
Member

Closing old inactive PR

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.

3 participants