Update http and url ECS fields by jalvz · Pull Request #1813 · elastic/apm-server

jalvz · 2019-01-18T10:31:07Z

loosen intake API for URL port to accept strings and numbers
add more tests and make sure to exercise all the fields

This includes all non indexed fields.

jalvz · 2019-01-18T10:31:48Z

Missing a few unit tests, putting it up for early review

jalvz · 2019-01-18T10:39:29Z

@elastic/apm-ui you need to be aware for this, non indexed fields root attributes are changing as per ECS:

{event}.context.request.url - > url
{event}.context.request (except url) - > http.request
{event}.context.response - > http.response

graphaelli · 2019-01-18T14:10:45Z

what's the plan with url.port ? I was expecting we would switch to a long for that in 7.0.

simitt · 2019-01-21T15:23:56Z

+
+- from: context.request.env
+  to: http.request.env
+  index: false


not defined in ECS

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.

@graphaelli do you have any opinion on this?

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.

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 👍

simitt · 2019-01-21T15:38:23Z

+  index: false
+
+- from: context.request.headers.user-agent
+  to: http.request.headers.user-agent


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.

simitt · 2019-01-21T15:54:26Z

+	delete(e.Context, "request")
+	delete(e.Context, "response")
+
+	utility.Add(fields, "context", e.Context)


If I understand this PR correct, then it aims for getting rid of context. What's left here?

simitt · 2019-01-21T15:55:15Z

+
 	events = append(events, beat.Event{Fields: fields, Timestamp: e.Timestamp})

 	return events


What happens with the context in span?

#1804 open to discuss

simitt

I suggest to also wait for @graphaelli 's opinion about those changes, but LGTM.

simitt · 2019-01-22T14:46:39Z

You need to (manually) update the approval files for the system-tests.

webmat · 2019-01-22T15:50:05Z

+
+  # Not in ECS
+- from: context.request.headers.user-agent
+  to: http.request.headers.user-agent.original


Actually user_agent is in ECS: https://github.com/elastic/ecs#user_agent

what is the reason for that? the actual header name is User-Agent

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

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 :-)

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.

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 :-)

Yes, they are present at the same time.

Also loosen the Intake API so that url.port can be a string or a number. It will be indexed as number

jalvz · 2019-01-23T16:09:00Z

@sqren this updates the elasticsearch/generated files again.

request.method is also lower case now, in case is relevant

graphaelli

minor feedback but lgtm

graphaelli · 2019-01-23T21:47:43Z

+       - name: scheme
+         type: keyword
+         description: >
+           The protocol of the request, e.g. "https:".


the example should be "https" - ecs is surprisingly specific on this - https://github.com/elastic/ecs#-url-fields.

graphaelli · 2019-01-23T21:50:28Z

                },
                "port": {
-                    "type": ["string", "null"],
+                    "type": ["string", "integer","null"],


jalvz force-pushed the ecs-http branch from 4603bd8 to b461078 Compare January 18, 2019 14:18

simitt added the [zube]: In Review label Jan 21, 2019

simitt reviewed Jan 21, 2019

View reviewed changes

watson mentioned this pull request Jan 21, 2019

Not working v2.2.0 elastic/apm-agent-nodejs#813

Closed

1 of 3 tasks

jalvz force-pushed the ecs-http branch 2 times, most recently from 53cc1fa to b3babfe Compare January 22, 2019 09:46

simitt approved these changes Jan 22, 2019

View reviewed changes

webmat reviewed Jan 22, 2019

View reviewed changes

jalvz force-pushed the ecs-http branch 2 times, most recently from 3ec8074 to 018b0a0 Compare January 23, 2019 14:23

Update http and url fields according to ECS

beec178

Also loosen the Intake API so that url.port can be a string or a number. It will be indexed as number

jalvz force-pushed the ecs-http branch from 018b0a0 to beec178 Compare January 23, 2019 15:48

graphaelli approved these changes Jan 23, 2019

View reviewed changes

jalvz merged commit af76d15 into elastic:master Jan 24, 2019

zube Bot added [zube]: Done and removed [zube]: In Review labels Jan 24, 2019


		events = append(events, beat.Event{Fields: fields, Timestamp: e.Timestamp})

		return events

Conversation

jalvz commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jalvz commented Jan 18, 2019

Uh oh!

jalvz commented Jan 18, 2019

Uh oh!

graphaelli commented Jan 18, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simitt left a comment

Choose a reason for hiding this comment

Uh oh!

simitt commented Jan 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jalvz commented Jan 23, 2019

Uh oh!

graphaelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jalvz commented Jan 18, 2019 •

edited

Loading