-
Notifications
You must be signed in to change notification settings - Fork 518
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
Update http and url ECS fields #1813
Conversation
Missing a few unit tests, putting it up for early review |
@elastic/apm-ui you need to be aware for this, non indexed fields root attributes are changing as per ECS:
|
what's the plan with |
|
||
- from: context.request.env | ||
to: http.request.env | ||
index: false |
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.
not defined in ECS
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.
The standard in ECS is that if the name does not end with text
then it is a keyword
. This is an object, so not sure how to bring this into ECS convention.
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.
@graphaelli do you have any opinion on this?
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.
The recommendation is to have all fields as keyword
. If you need text
datatype, you should have it as a multi-field. This is the reverse of the ES default.
Since this field is not in ECS, you're free to do what you want here.
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.
And about things that aren't in ECS (like http.request.env
), they're not automatically a problem. If we don't add it to ECS, there will never be a conflict. And in this case, it's perfectly fine for you to define this field there.
http.request.env
is the variables defined in the application's context? If that's the case, I can guarantee it'll never be in ECS (as it has nothing to do with the HTTP protocol), and would not named like this (as we try to use full words as much as possible).
So no conflict here 👍
_meta/ecs-migration.yml
Outdated
index: false | ||
|
||
- from: context.request.headers.user-agent | ||
to: http.request.headers.user-agent |
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.
ECS defines user_agent
. While it is a root field there and not here, for the purpose of aligning I suggest to use http.request.headers.user_agent.original
.
beater/test_approved_es_documents/TestPublishIntegrationErrors.approved.json
Show resolved
Hide resolved
delete(e.Context, "request") | ||
delete(e.Context, "response") | ||
|
||
utility.Add(fields, "context", e.Context) |
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.
If I understand this PR correct, then it aims for getting rid of context
. What's left here?
delete(e.Context, "response") | ||
|
||
utility.Add(fields, "context", e.Context) | ||
|
||
events = append(events, beat.Event{Fields: fields, Timestamp: e.Timestamp}) | ||
|
||
return events |
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.
What happens with the context
in span
?
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.
#1804 open to discuss
53cc1fa
to
b3babfe
Compare
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.
I suggest to also wait for @graphaelli 's opinion about those changes, but LGTM.
You need to (manually) update the approval files for the |
|
||
# Not in ECS | ||
- from: context.request.headers.user-agent | ||
to: http.request.headers.user-agent.original |
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.
Actually user_agent
is in ECS: https://github.com/elastic/ecs#user_agent
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.
what is the reason for that? the actual header name is User-Agent
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.
I think this should stay as user-agent
since it's nested under http.request.headers
but I'd also be ok to switch if everyone else prefers that
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.
In ECS it's at the top level, so not really a conflict with how you normalize header names as field names (if you decide to move it at the top level).
If you decide to leave it nested here because you prefer to list it as one of the header entries, then I'd opt to leave as is, to keep the consistent usage of '-' with other headers.
My actual suggestion would be to move this to ECS, however (and I thought I had seen another entry in this file to do so).
Moving this field to the ECS name will ensure the user agents recorded by APM can be found easily by any integration or person that supports the common schema, without having to create an exception for APM. That's the whole point of ECS, after all :-)
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.
Yes, as you noted there is another entry in this file for that. We have 2 user agents: the one in the request to the apm-server (which is moved to the ECS field at the root) and this one, which is in a request possibly captured by an APM agent.
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.
Are these present in the same event at the same time? If not, then both should break down the user agent in the ECS field, user_agent
at the top level.
If they can both be present in the same event, then that's a more complicated question :-)
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.
Yes, they are present at the same time.
3ec8074
to
018b0a0
Compare
Also loosen the Intake API so that url.port can be a string or a number. It will be indexed as number
@sqren this updates the
|
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.
minor feedback but lgtm
- name: scheme | ||
type: keyword | ||
description: > | ||
The protocol of the request, e.g. "https:". |
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.
the example should be "https" - ecs is surprisingly specific on this - https://github.com/elastic/ecs#-url-fields.
@@ -186,7 +186,7 @@ const ModelSchema = `{ | |||
"maxLength": 1024 | |||
}, | |||
"port": { | |||
"type": ["string", "null"], | |||
"type": ["string", "integer","null"], |
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.
good idea
This includes all non indexed fields.
Fixes #1752