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

Allow overriding of meta data in intake V2 #1175

Closed
felixbarny opened this issue Jul 24, 2018 · 18 comments · Fixed by #1901
Closed

Allow overriding of meta data in intake V2 #1175

felixbarny opened this issue Jul 24, 2018 · 18 comments · Fixed by #1901
Assignees
Milestone

Comments

@felixbarny
Copy link
Member

In Java, you can have multiple applications in one JVM and multiple frameworks in one application. Therefore, I propose that the intake API should allow that specific payload attributes of service can be overridden. Related Java agent issue: elastic/apm-agent-java#136

The payload JSON schema defines meta-data which is valid for all the transactions and spans it contains. Instead of allowing only some attributes like service.name to be overridden in a Transaction, add the service object to Transaction, Span and Error. This allows default values to be set in the Payload "header" but allows specific objects to override the defaults. In languages where there can only ever be one service.name, this requires no changes and no additional overhead.

One problem with that approach is that there are required attributes in service like agent and name. These should not have to be repeated. If possible with JSON schema, it would be nice to remove the required part from the service definition and only add it where necessary whenever the service is used.

@axw
Copy link
Member

axw commented Oct 2, 2018

I was looking into elastic/apm-agent-go#69 today, seeing what needed to be changed in the server. Turns out it already works, probably by accident :)

The server merges metadata.service with {transaction,span,error}.context.service, with fields in the former (service-level) overriding the latter (transaction-level):

func (m *Metadata) Merge(eventContext common.MapStr) common.MapStr {
eventContext = m.normalizeContext(eventContext)
utility.Add(eventContext, "system", m.systemFields)
utility.Add(eventContext, "process", m.processFields)
utility.MergeAdd(eventContext, "user", m.userFields)
utility.MergeAdd(eventContext, "service", m.serviceFields)
return eventContext
}

This is backwards for what @felixbarny is requesting, but works in my case where I just want to set the framework at the transaction level. I think we'll want to swap the merge order.

@simitt
Copy link
Contributor

simitt commented Oct 2, 2018

related Issue #1255

@alvarolobato alvarolobato changed the title Allow overriding of meta data in intake API v2 Allow overriding of meta data Oct 15, 2018
@alvarolobato alvarolobato changed the title Allow overriding of meta data Allow overriding of meta data in intake V2 Oct 15, 2018
@alvarolobato alvarolobato added this to the 6.6 milestone Nov 12, 2018
@alvarolobato alvarolobato self-assigned this Nov 20, 2018
@alvarolobato alvarolobato removed this from the 6.6 milestone Dec 11, 2018
@simitt
Copy link
Contributor

simitt commented Jan 28, 2019

@elastic/apm-agent-devs could you please define which fields you'd like to send on an event level.

We need to agree on whether the information per event should be merged with the object sent in the metadata or should be set instead of the metadata information.

At the moment the user can be set per event. In case a user is given in the metadata and per event, the information from the event is stored, and the information from the metadata is ignored for this user.

@felixbarny
Copy link
Member Author

I'd like to send service fields on an event level. I'd prefer to only set values which differ from the meta data. I guess that means to merge.

@gregkalapos
Copy link
Contributor

I'd like to send service fields on an event level.

+1 from .NET on service.

@simitt
Copy link
Contributor

simitt commented Jan 28, 2019

If we merge the data, it means you need to ensure to send all attributes you want to be overwritten in the event. E.g. if the user sent within metadata is a different one than the one sent in the event (for whichever reason this should ever happen), you would need to send all user data in the event, as otherwise two unrelated user objects would be mixed. Same for every other object you want to overwrite.
To me this looks more error prone than setting either the event object or the metadata object. I do see your point sending as little information as possible though, so both options are fine with me. I just wanted to point out the potential risk.

@jalvz
Copy link
Contributor

jalvz commented Jan 28, 2019

fwiw, the least complicated to address this imo is:

  • define all attributes in the events (service, system, etc) - this way event types are fully defined in isolation.
  • treat metadata as a bag of defaults of field and/or objects (ie if any field is not set in the event, grab it from the metadata)
  • ideally, metadata would explicitly define which types are the defaults for, eg. a list of objects like {field: context.user, value: {id:123, email:whatev}, types: [error, transaction]} but probably we can't change this on time for 7.0 any ways.

@felixbarny
Copy link
Member Author

What would be a use case to set the user in the meta data?

The use cases I have in mind are overriding the service name and the framework name on a per-event basis. I don't think these would be breaking changes.

If there is a use case to set data which is usually associated with transactions or spans, like user, in the meta data, I feel like we should discuss that separately.

@simitt
Copy link
Contributor

simitt commented Jan 28, 2019

What would be a use case to set the user in the meta data?

The API already allows sending a user in the metadata since 6.5. I assume sending user data via metadata makes especially sense for the rum agent, for sending information about the currently logged in user.

@simitt
Copy link
Contributor

simitt commented Jan 28, 2019

I suggest we implement the same solution for whichever data are given in the metadata and can be overriden per event, so there is a clear definition of the outcome of overriding data.

@axw
Copy link
Member

axw commented Jan 29, 2019

@elastic/apm-agent-devs could you please define which fields you'd like to send on an event level.

I currently only need to set service.framework.

@simitt
Copy link
Contributor

simitt commented Feb 6, 2019

I am going to add service.framework to context on a per event type as a bugfix, since this is working in 6.x.
@gregkalapos and @felixbarny are there other parts from service that make sense for your agents to send up on a per event basis?
For now I'd like to avoid just adding all metadata or service fields without a real use case. As soon as the code is there we need to maintain it and overriding fields is error prone as we saw in the past.

@simitt simitt assigned simitt and unassigned alvarolobato Feb 6, 2019
@simitt simitt added this to the 7.0 milestone Feb 6, 2019
@simitt simitt added the bug label Feb 6, 2019
@felixbarny
Copy link
Member Author

service.name as well. It would be great if service.name could be made optional in the meta data in case it's set per event. See also the first post of this thread.

@gregkalapos
Copy link
Contributor

service.name as well. It would be great if service.name could be made optional in the meta data in case it's set per event. See also the first post of this thread.

Same from me. I think service.name is more important... the framework will be probably the same, but with different service names - at least in .NET that would be very typical.

@simitt
Copy link
Contributor

simitt commented Feb 7, 2019

@felixbarny I can see why making service.name and service.agent optional here makes sense
I am going to drop all requirements for which fields need to be set on the context.service.

@felixbarny
Copy link
Member Author

But it probably still makes sense to check if the resulting document has service.name set. I don't think there is a need to make service.agent overridable or optional though.

@simitt
Copy link
Contributor

simitt commented Feb 7, 2019

Giving it a second thought I am not going to only define some service attributes to be overridable, as this would for probably lead to more discussion and confusion in the future.

All the requirements for sending up service data on the metadata level stay the same. Every attribute of service can be overriden when sent in context, where all restrictions regarding optional and required information are dropped.

@felixbarny
Copy link
Member Author

Gotcha, I'm fine with that

simitt added a commit to simitt/apm-server that referenced this issue Feb 7, 2019
simitt added a commit that referenced this issue Feb 8, 2019
* Allow overriding `service` information from `context`.

fixes #1175
simitt added a commit to simitt/apm-server that referenced this issue Feb 11, 2019
* Allow overriding `service` information from `context`.

fixes elastic#1175
simitt added a commit to simitt/apm-server that referenced this issue Feb 11, 2019
* Allow overriding `service` information from `context`.

fixes elastic#1175
simitt added a commit that referenced this issue Feb 11, 2019
* Allow overriding `service` information from `context`.

fixes #1175
simitt added a commit to simitt/apm-server that referenced this issue Feb 11, 2019
* Allow overriding `service` information from `context`.

fixes elastic#1175
simitt added a commit that referenced this issue Feb 14, 2019
* Allow overriding `service` information from `context`.

fixes #1175
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants