-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix for #1382 - logs are not easily queryable with es storage #1383
Conversation
As logs was not defined as a nested object. Nested elastic query did not work for logs.fields either. This commit fixes this issue.
Jaeger backend can query for them, what's different there? @pavolloffay is this a breaking change? |
Maybe it is ES 6 related. Without the nested type for logs filed, the nested query did not work. I assume jaeger backend just uses the json returned for each span document, and renders it on the UI. I am not aware of any log browser other than the trace view. |
@istvanszoboszlai We have to make sure this works with ES5 and ES6. Do not merge I will have a more close look at this. |
I have tested the current master (without this PR) with the following query and everything works with ES 5.x and 6.x, therefore I am not sure if we need this change. The data has been generated with hotrod example.
Example span
The output of query from ES: |
@@ -64,7 +64,9 @@ | |||
"flags":{ | |||
"type":"integer" | |||
}, | |||
"logs":{ | |||
"logs" : { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I am fine to add the type: "nested"
as it is really a nested type.
https://www.elastic.co/guide/en/elasticsearch/reference/current/nested.html
The nested type is a specialised version of the object datatype that allows arrays of objects to be indexed in a way that they can be queried independently of each other.
"logs" : [
{
"timestamp" : 1561112938308666,
"fields" : [
{
"key" : "event",
"type" : "string",
"value" : "GetConn"
}
]
},
{
"timestamp" : 1561112938308674,
"fields" : [
{
"key" : "event",
"type" : "string",
"value" : "GotConn"
}
]
},
{
"timestamp" : 1561112938308721,
"fields" : [
{
"key" : "event",
"type" : "string",
"value" : "WroteHeaders"
}
]
},
{
"timestamp" : 1561112938308724,
"fields" : [
{
"key" : "event",
"type" : "string",
"value" : "WroteRequest"
}
]
},
{
"timestamp" : 1561112938377614,
"fields" : [
{
"key" : "event",
"type" : "string",
"value" : "GotFirstResponseByte"
}
]
},
{
"timestamp" : 1561112938377752,
"fields" : [
{
"key" : "event",
"type" : "string",
"value" : "PutIdleConn"
}
]
},
{
"timestamp" : 1561112938377812,
"fields" : [
{
"key" : "event",
"type" : "string",
"value" : "ClosedBody"
}
]
}
],
I have submitted #1622 as this PR needs to be signed and we want to release soon. |
Done in #1622. @istvanszoboszlai thanks for reporting the issue and submitting the PR. |
Thank you @pavolloffay for taking care of this. |
As logs was not defined as a nested object. Nested elastic query did not work for logs.fields either. This commit fixes this issue:
resolves #1382
Which problem is this PR solving?
#1382 - Logs are not easily queryable from elasticsearch (ES6)
Short description of the changes
Added two lines to elastic jaeger span index definition - see in changes.
Note
I do not know how to create unit tests for this. If unit tests are needed for this PR and there is someone who can guide me a bit, I would really appreciate that.