Add ability to set multiple tags at the same time about apm-agent-nodejs HOT 14 CLOSED

The object + set implies full overwrite instead of merge to me

from apm-agent-nodejs.

If you make setTag an alias for setTags({ key: value }) I see no reason to deprecate it.

will calling setTags multiple times just add to the list of tags?

I would guess that I would expect this behaviour but maybe it should be addTags then. what happens now if you call setTag twice?

from apm-agent-nodejs.

@AdriVanHoudt calling setTag twice with the same key overwrites the old value. Calling it with different keys just sets the values for those, but still keeps the other tags

from apm-agent-nodejs.

I see, maybe this makes more sense then

agent.setTags([key1, 'value'], [key2, 'value'])
// or
agent.setTags([[key1, 'value'], [key2, 'value']])

from apm-agent-nodejs.

@AdriVanHoudt what's your use-case for using arrays to specify key/value pairs instead of a flat object?

from apm-agent-nodejs.

cc @elastic/apm-agent-devs

from apm-agent-nodejs.

I'd go with a plain object. That array form seems a bit less friendly to the end-user, and you can get that with Object.entries() anyway, if you really need it.

from apm-agent-nodejs.

Maybe keep the way it works but call it assign or merge as that is what it actually does - JavaScript-wise?

from apm-agent-nodejs.

There seems to be a consensus that:

1. It should be possible to assign tags to a transaction multiple times during the transaction lifetime
2. It should be possible to overwrite a specific tag with a new value
3. Setting tags a second time during the same transaction, shouldn't remove the tags set previously on the same transaction
4. The function name setTags doesn't convey the intend of point no 3

I'm not sure that I like assignTags. Not sure why, but it just seems weird to me even though that's what used for Object.assign etc. I personally prefer addTags better, and I think that it's fairly obvious that calling addTags twice with the same key, will overwrite the value of said key.

So how about this:

// starting out, there's no tags at all

agent.addTags({tag1: 'foo', tag2: 'bar'}) // now there's two tags: tag1 + tag2

agent.addTags({tag1: 42, tag3: 'baz'}) // now there's three tags: tag1 + tag2 + tag3

You could argue that it's not obvious that the tags set are scoped to the current transaction and that you should spell it out like agent.setTransactionTags()?

What do you think of this (either setTags or setTransactionTags)?

from apm-agent-nodejs.

I haven't implemented it yet, but I'm considering if tags set during a transaction should also be applied to exceptions/messages that happen during that transaction. There isn't really a way to tag "unhandled" exceptions in any other way AFAICT. In that case, setTransactionTags would be a bit of a misnomer.

from apm-agent-nodejs.

I wonder how reasonable it'd be to have something like agent.currentTransaction so one could do agent.currentTransaction.setTags(...) and have it apply to that transaction. In that way, it's clear it applies specifically to the transaction object and also provides a mechanism for further transaction-specific interfaces.

from apm-agent-nodejs.

@beniwohli Brainstorming: How if we had some way to simply link errors to a transaction? E.g. attach the transaction id of the transaction that was active when the error occurred along with the error

@Qard Good idea. I've been considering something similar in other scenarios. The reason why I've so far not implemented something like that is because I haven't found a good way to handle the case where there isn't a current transaction. I don't want to add if (agent.currentTransaction) ... all over the place. I've been thinking about defining it as getter property so it can return a dummy transaction if none is there, but that also have its own set of problems. But I haven't spent too much time trying to solve this, so there might be a good way that I haven't thought of.

from apm-agent-nodejs.

Maybe some sort of get-or-create thing? Something like agent.getOrStartTransaction(...) could work.

from apm-agent-nodejs.

@Qard I think with for instance addTags() we'll not want to do anything if there isn't currently an active transaction. It could be that instrumentation have been disabled, or we're sampling and this current request hasn't been chosen as a sample, or simply that there's a bug where we have lost context due to an un-patched async operation. We have have the same issue with agent.buildSpan() btw, this will return null instead of a span if there isn't an active transaction so all subsequent calls to span.* needs to have a guard before it. So it would be cool if we could find a solution to this that made the API simpler to work with 😃

from apm-agent-nodejs.