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

Read trace ID from ES without leading zeros #1

Merged
merged 2 commits into from
Dec 3, 2019
Merged

Read trace ID from ES without leading zeros #1

merged 2 commits into from
Dec 3, 2019

Conversation

pavolloffay
Copy link

@pavolloffay pavolloffay commented Dec 3, 2019 •
edited
Loading

@pavolloffay
Read trace ID from ES without leading zeros
1b5f0bc 
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
yurishkuro
yurishkuro reviewed Dec 3, 2019
View reviewed changes
plugin/storage/es/spanstore/reader.go Outdated
legacyTraceID = fmt.Sprintf("%x", traceID.Low)
} else {
legacyTraceID = fmt.Sprintf("%x%016x", traceID.High, traceID.Low)
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think we only want to do that if the normal ID string starts with a 0, otherwise issue a single query.

plugin/storage/es/spanstore/reader.go Outdated
legacyTraceID = fmt.Sprintf("%x%016x", traceID.High, traceID.Low)
}
query := elastic.NewBoolQuery().
Should( elastic.NewTermQuery(traceIDField, traceID.String()).Boost(2), elastic.NewTermQuery(traceIDField, legacyTraceID))
Copy link
Owner

Choose a reason for hiding this comment

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

I am not really sure what goes on in this query, but is there any reason to not simply add the legacy ID to searchRequests?

Copy link
Author

Choose a reason for hiding this comment

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

It's basically or statement and the new ID has a higher priority https://www.elastic.co/guide/en/elasticsearch/reference/6.6//query-dsl-term-query.html

Copy link
Author

Choose a reason for hiding this comment

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

I would rather do this than running a separate query. It might be more effective.

@pavolloffay
Review comments
4acc64f 
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
yurishkuro
yurishkuro approved these changes Dec 3, 2019
View reviewed changes
Copy link
Owner

@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.

I can fix in my branch

plugin/storage/es/spanstore/reader.go
traceIDsModels[i] = traceID
if _, ok := traceIDsMap[traceID]; !ok {
traceIDsMap[traceID] = true
traceIDsModels[i] = traceID
Copy link
Owner

Choose a reason for hiding this comment

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

This may leave gaps in the final array. We need to allocate it with 0 size and use append()

Copy link
Author

Choose a reason for hiding this comment

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

would that cause issues? I wanted to avoid reallocation.

Copy link
Owner

Choose a reason for hiding this comment

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

it's still pre-allocated to the max possible length, so no reallocation

plugin/storage/es/spanstore/reader_test.go
spanDate := time.Now().UTC().Format("2006")
fmt.Println(spanDate)
func TestTraceIDQuery(t *testing.T) {
uintMax := uint64((1 << bits.UintSize) - 1)
Copy link
Owner

Choose a reason for hiding this comment

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

this is only 32bits. I think you want ^uint64(0)

@yurishkuro yurishkuro marked this pull request as ready for review December 3, 2019 16:53
@yurishkuro yurishkuro merged commit 7e561d2 into yurishkuro:fix-1578 Dec 3, 2019
yurishkuro pushed a commit that referenced this pull request Aug 18, 2023
@albertteoh
Add the enable tracing opt-in flag (jaegertracing#4685)
2119f5f 
## Which problem is this PR solving?
Resolves jaegertracing#4680

## Description of the changes
- Add an opt-in option `--query.enable-tracing` to enable tracing for
the jaeger-query component.
- The jaeger all-in-one component does not expose this flag since traces
are emitted to port 4317 by default, which all-in-one listens on.

## How was this change tested?

```
# Run jaeger-query component with tracing enabled and verify that the connection errors are appearing in stdout.
$ SPAN_STORAGE_TYPE=memory go run -tags ui ./cmd/query/main.go --query.enable-tracing
...
{"level":"info","ts":1692363754.9049716,"caller":"grpc/clientconn.go:1301","msg":"[core][Channel #1 SubChannel #2] Subchannel Connectivity change to CONNECTING","system":"grpc","grpc_log":true}
{"level":"info","ts":1692363754.9050152,"caller":"grpc/clientconn.go:1414","msg":"[core][Channel #1 SubChannel #2] Subchannel picks a new address \"localhost:4317\" to connect","system":"grpc","grpc_log":true}
{"level":"warn","ts":1692363754.9058733,"caller":"grpc/clientconn.go:1476","msg":"[core][Channel #1 SubChannel #2] grpc: addrConn.createTransport failed to connect to {Addr: \"localhost:4317\", ServerName: \"localhost:4317\", }. Err: connection error: desc = \"transport: Error while dialing: dial tcp 127.0.0.1:4317: connect: connection refused\"","system":"grpc","grpc_log":true}
{"level":"info","ts":1692363754.9067123,"caller":"grpc/clientconn.go:1303","msg":"[core][Channel #1 SubChannel #2] Subchannel Connectivity change to TRANSIENT_FAILURE, last error: connection error: desc = \"transport: Error while dialing: dial tcp 127.0.0.1:4317: connect: connection refused\"","system":"grpc","grpc_log":true}
...

# Run jaeger-query component with tracing disabled and verify that the connection errors no longer appear.
# Of course, we can't see traces in Jaeger UI because there's nothing to receive the traces.
$ SPAN_STORAGE_TYPE=memory go run -tags ui ./cmd/query/main.go

# Start an all-in-one instance just as a quick and dirty way to bring up an in-memory jaeger stack to
# receive traces from jaeger-query
$ make run-all-in-one

# Run jaeger-query as a separate component, listening on different ports to all-in-one to avoid port binding collisions.
$ SPAN_STORAGE_TYPE=memory go run -tags ui ./cmd/query/main.go --query.enable-tracing --query.grpc-server.host-port :17685 --query.http-server.host-port :17686 --admin.http.host-port :17687

# Open localhost:17686 in a browser and refresh a few times to emit traces to jaeger all-in-one.
```

Confirmed that `jaeger-query` is visible and contains traces:

<img width="1572" alt="Screenshot 2023-08-18 at 11 45 26 pm"
src="https://github.com/jaegertracing/jaeger/assets/26584478/a348e804-6d37-49f9-9d9f-f73854e9b6bc">


## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
~- [] 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`: `yarn lint` and `yarn test`

---------

Signed-off-by: albertteoh <see.kwang.teoh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pavolloffay @yurishkuro