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

[agents] Labels [formerly tags] management API #42

Closed
7 tasks done
graphaelli opened this issue Jan 29, 2019 · 13 comments
Closed
7 tasks done

[agents] Labels [formerly tags] management API #42

graphaelli opened this issue Jan 29, 2019 · 13 comments

Comments

@graphaelli
Copy link
Member

graphaelli commented Jan 29, 2019

context.tags will be stored in labels with the next major version of apm-server per elastic/apm-server#1826. There is still an open question of whether to update the intake API as well, tracked in elastic/apm-server#1835, but that's outside of the scope of this issue.

Here, let's decide if agents would like to [not] add/replace existing tag management api calls with those include label, eg

Proposal: alias functions substituting label for tag soon, marking tag deprecated for release in agents post 7.0 stack release (could even be before if docs can be suppressed). Wait until the next major of each agent after that point to remove the tag functions.

Per usual, please vote 👍 or 👎 for adopting this proposal and add any questions/comments. Checkboxes for implementation per-agent will be added if the proposal is accepted. We can also take this opportunity to sync apis across agents.

Please keep in mind whether these changes conflict with reserved words in each language.


This can be released by agents at any time but but some time near 7.0.0 to reduce confusion with tags ending up stored as labels.

@elastic/apm-agent-devs please link your implementation issues

@felixbarny
Copy link
Member

@elastic/apm-agent-devs Did you already make the move? The Java agent has deprecated addTag in favor of addLabel and released in version 1.5.0.

Let's make sure to link the implementation issues so we can close this one 🙂

@bmorelli25
Copy link
Member

Adding in a question to this issue: Is there any plan to standardize on how these new API endpoints work in the agents? For example, some agents convert all tag values to strings before sending them to the APM server. This means if I add an agent that accepts strings, numbers, or booleans as a tag value, it doesn't matter, because the values will be coerced to strings when they hit Elasticsearch.

@felixbarny
Copy link
Member

The support for Number and boolean tags was added somewhat recently so probably some agents don't offer those yet. I don't think there are agents which don't support numbers and booleans on purpose. Probably they didn't see the need just yet. But I do think it makes sense to eventually align.

@Qard
Copy link

Qard commented Apr 23, 2019

I would consider using that support to be a breaking change as it would mean the agent would no longer work with versions of APM Server prior to that support being added.

@felixbarny
Copy link
Member

That's a very good point @Qard! Using number tags with an APM Server prior to 6.7 would result in schema violations. However, I think it's ok to add those methods and add a warning to the documentation to only use them on APM Server versions 6.7+ (which I will do now 😇).

@hmdhk
Copy link
Contributor

hmdhk commented Apr 25, 2019

Since we are adding a new method addLabel we can also start supporting additional value types for tags. However, I agree that this should be considered a breaking change since we can no longer say we are fully compatible with versions below 6.7 of the APM server.

@felixbarny , I think having exceptions to what we support with version of APM server makes things harder to keep track of.

@watson
Copy link
Contributor

watson commented May 8, 2019

This means if I add an agent that accepts strings, numbers, or booleans as a tag value, it doesn't matter, because the values will be coerced to strings when they hit Elasticsearch.

So even if we stop converting tag/label values to strings in the agents, Elasticsearch will just do it for us anyway after they have been sent up? This is news to me. If so, what was the reason for adding support for this in the first place? As a user of a dynamically typed language, being able to see the type of value is a nice feature 🙂

@felixbarny
Copy link
Member

So even if we stop converting tag/label values to strings in the agents, Elasticsearch will just do it for us anyway after they have been sent up?

That's only true for the current index. For example, if you used to send up "labels.foo"="42", the type of labels.foo is keyword. If you then send "labels.foo"=42, ES will coerce the number to a string so that the field type is still keyword. When the index is rolled over (typically the next day), sending "labels.foo"=42 leads to labels.foo being typed as scaled_float.

See also https://github.com/elastic/apm-server/blob/41fae6d2db1e54b39d199375cd304f20ee23f496/_meta/fields.common.yml#L121-L131

@felixbarny
Copy link
Member

But that means there is a race condition. Whichever agent gets to index a tag for the first time in a specific index determines the type of the field. That's especially a problem when not all agent support numbers and users use the same tag names across different language stacks. I guess that's what @bmorelli25 meant.

@bmorelli25
Copy link
Member

Yes, sorry, my initial comment wasn't very clear. Felix explains it better ^

@mikker
Copy link
Contributor

mikker commented May 8, 2019

I've really grown to like the idea of having three separate methods, set_string_label(key, value), set_number_label(key, value) and set_boolean_label(key, value) to avoid confusion. set_label(key, value) could call through to set_string_label and it wouldn't even be a breaking change.

As the type of the field is determined by the first value I think it's a good thing to have the user be explicit in their intent.

@graphaelli
Copy link
Member Author

graphaelli commented May 8, 2019

We should be able to utilize ignore_malformed on labels if elastic/elasticsearch#12366 is implemented. I've confirmed the current setting does not work on the data types currently allowed for labels.

@alvarolobato
Copy link
Contributor

Closing now that we have tracking issues for all the agents.

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

No branches or pull requests

8 participants