elastic / apm-agent-nodejs

https://www.elastic.co/guide/en/apm/agent/nodejs/current/index.html
BSD 2-Clause "Simplified" License
582 stars 224 forks source link

Log hapi log tags as the new dedicated tags #362

Open AdriVanHoudt opened 6 years ago

AdriVanHoudt commented 6 years ago

Is your feature request related to a problem? Please describe. Right now it logs under context.custom.tags while there is a dedicated context.tags https://github.com/elastic/apm-agent-nodejs/blob/cbf39df9ebd68d29f136cfeb40cb5b6f8b099073/lib/instrumentation/modules/hapi.js#L91

Describe the solution you'd like Log it to context.tags so I can query on it 🎉

Describe alternatives you've considered in a filter do

if (item.exception && item.context.custom.tags) {
    item.context.tags = { ...item.context.tags, ...item.context.custom.tags };
}

Additional context Add any other context or screenshots about the feature request here.

watson commented 6 years ago

Great idea @AdriVanHoudt 😃

The tags under context.tags have certain restrictions to make them queryable:

Would that be ok in regards to hapi tags?

Related issue: #11

AdriVanHoudt commented 6 years ago

Keys should be strings anyways https://hapijs.com/api#-serverlogtags-data-timestamp Values can indeed be objects and stringify seems like a nice solution 👌 Would you do the periods to underscores thing for all the tag keys or only for hapi?

watson commented 6 years ago

Dots in field names have special meaning in Elasticsearch, where 'foo.bar': 'baz' essentially is turned into something like 'foo': { 'bar': 'baz' }. So to avoid any side-effects of this, we need to make sure the keys do not contain dots. One easy way to achieve this would be to replace dots with something else, like an underscore.

AdriVanHoudt commented 6 years ago

yeah all for it. My question was more towards do you want this in the hapi plugin or somewhere more general so that it has the same behaviour when doing .addTags or .setTag ^^

watson commented 6 years ago

I don't have enough experience with hapi to be comfortable answering that unfortunately.

But I was never really a fan of having it under "custom". It was only there because that was the only place we had available for that sort of data in the Opbeat days. But the agent really shouldn't put anything under "custom" as I see that as being reserved for data added by you the user - not something the agent automatically should populate.

If we removed it from "custom", how hard would it be for you as a user to manually call addTags with the tags vs. if the agent did it for you. I'm trying to weigh the complexity of us having to maintain a feature like this vs how easy it is for the user to manually do it.

AdriVanHoudt commented 6 years ago

If I do apm.addTags now with a name containing a string, what does it do now? Throw? I don't think it would be hard to call addTags myself but a user would quickly forget to check on the periods. For me personally maintaining it outweighs having the user do it. It is always awesome when "things just work". I also checked our logging and afaik we never log with tags with a period (or a special character for that matter). I would find it odd if someone else did but you know :D

watson commented 6 years ago

Today the APM Server would reject the data if the tags contained a period (or a * or a "). So the user would have to manually replace those. In that light it would make sense if the agent took care of it I guess. Let's update the current code to use addTags instead and replace the illegal chars with _.