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

Bring to parity with metrics and logs hints-based auto-discovery #28

Open
1 of 3 tasks
eyalkoren opened this issue May 31, 2022 · 18 comments
Open
1 of 3 tasks

Bring to parity with metrics and logs hints-based auto-discovery #28

eyalkoren opened this issue May 31, 2022 · 18 comments

Comments

@eyalkoren
Copy link
Contributor

eyalkoren commented May 31, 2022

Context: elastic/beats#23876

It’ll be great if our APM integration would follow as similar patterns as possible to logs and metrics integrations.
A non-exhaustive list of related items includes:

  • Following the same naming pattern, with a co.elastic.traces prefix, followed by / and sub configuration if required
  • Supporting Docker labels as well as k8s pod annotations
  • Copy over the similar documentation and once GA, integrate with the other Elastic Agent hint-based auto-discovery documentation

List of discussed naming patterns:

suggestion comment actively discussed
     
co.elastic.traces: java too narrow (traces, java) no
co.elastic.traces/java: true too narrow (traces, java) no
co.elastic.traces/agent: java avoid usage of agent for ambiguity with elastic-agent no
co.elastic.traces/language too narrow (traces) no
co.elastic.traces/apm-agent too narrow (traces) no
co.elastic.apm/inject-apm-agent: java too verbous?  
co.elastic.apm/auto-instrument    
co.elastic.apm/* should apm even be mentioned? yes
co.elastic.instrumentation/*   yes
co.elastic.apm/attach: agent stay consistent with the helm chart naming yes
@felixbarny
Copy link
Member

How would the equivalent to elastic-apm-agent: java look like? co.elastic.traces: java or co.elastic.traces/java: true?

@eyalkoren
Copy link
Contributor Author

Maybe something like co.elastic.traces/agent: java would fit the naming pattern better...

@stuartnelson3
Copy link
Contributor

naming pattern updated in 973a447

@simitt
Copy link
Contributor

simitt commented Jun 1, 2022

this might be nitpicking, but I'm not convinced that the suggested naming of co.elastic.traces/agent: java is ideal. Is the term agent considered unambiguous (not to be confused with elastic-agent) because of co.ealstic.traces ?

@eyalkoren
Copy link
Contributor Author

Good question. Maybe co.elastic.traces/language or co.elastic.traces/apm-agent would be better.
Maybe even the term traces is not the best to reflect APM data.
I don't feel I am in a good position to make a call. Maybe PMs or SAs may have better insights.

BTW, this issue is currently only about hint-based auto-discovery because of how the first webhook version works. I think that ideally we should support any other type of auto-discovery that logs and metrics support.

@felixbarny
Copy link
Member

Good points, Silvia. Another idea: co.elastic.apm/inject-apm-agent: java

@stuartnelson3
Copy link
Contributor

All of the suggestions are (to me) implying that the configuration differs only on the type of agent used for auto-instrumentation (language, apm-agent, inject-apm-agent). I had in mind the actual configs would vary along the environment variables as well. This is a vague way of saying I think something like co.elastic.apm/auto-instrument might be better since it doesn't have the agent bias present in the other names.

@ruflin
Copy link

ruflin commented Jun 9, 2022

++ on the idea of having these patterns also for apm. I'm currently working on a doc around setting a high level vision on how Elastic Agent in k8s should work. One of the main ideas in there is to have a k8s-fleet-operator. Now having these labels, the fleet-operator could take over doing the auto-instrumentation and if any of the containers has a label that other apm-agents are used, automatically setup the apm-server and inject the proper configurations (environment variables?) into the container to make sure data is delivered to the local apm-server.

@felixbarny
Copy link
Member

So users don't even need to actively add or configure the APM integration? That sounds fantastic!

Would the fleet operator be able to leverage this webhook for attachment by automatically installing it when it discovers an co.elastic.apm/* annotation/label?

@ruflin
Copy link

ruflin commented Jun 9, 2022

Yes, exactly. But keep in mind, this is all brainstorming so far.

@simitt
Copy link
Contributor

simitt commented Sep 9, 2022

@axw @felixbarny @marclop and I have picked this up again and I added a table with proposed names to the description. Let's keep that up-to date while we continue the conversation.

@alex-fedotyev and @chrisdistasio can you please chime in and help defining the naming.

@simitt simitt added this to the GA milestone Sep 9, 2022
@felixbarny
Copy link
Member

As we're thinking to rename the helm chart to apm-attacher (see #52), co.elastic.apm/attach: <agent> would feel consistent.

@felixbarny
Copy link
Member

felixbarny commented Sep 14, 2022

Last call for objections: If there are no objections by tomorrow, we'll change the annotation to co.elastic.apm/attach: <agent>. To make updating for early adopters more seamless, let's also support the old annotation (which we can crop before going GA).

@simitt
Copy link
Contributor

simitt commented Sep 14, 2022

I like the suggestion of co.elastic.apm/attach: <agent>. It is in line with the latest image/helm chart proposal in #52

@felixbarny @axw @Mpdreamz @AlexanderWert @alex-fedotyev @chrisdistasio @graphaelli any concerns about the suggested name?
If not, I suggest we move forward with this by tomorrow, to unblock Marc from recording the screencast with the new names.

@ruflin
Copy link

ruflin commented Sep 14, 2022

My initial thought reading this again was aligning naming the the types in the data stream naming scheme. But an APM Agent can send data too all different types. Because of this I like that apm is not even part of the types and I think it fits well. 👍 on my end.

Pinging @gizas also on this from the cloud native team to make sure it does not conflict and align with other autodisocvery concerns.

@gizas
Copy link

gizas commented Sep 14, 2022

Hello all,

This is the current WIP: elastic/elastic-agent#662
So indeed our format remains co.elastic.hints/*.

I dont see any conflict at the moment and I like as well the suggestion of co.elastic.apm/attach: .

(Will need some more time to review the whole context but on the other hand feel free to comment on above 662 as well)

@simitt
Copy link
Contributor

simitt commented Jan 16, 2024

@trentm @estolfo regarding the remaining steps for making the attacher GA, I'd suggest to remove the legacy annotation co.elastic.traces/agent before GA.
Additional support of labels and annotations can be added later, but removing the annotation would be a breaking change after GA.

@trentm
Copy link
Member

trentm commented Jan 17, 2024

The legacy annotation is dropped in #98. Additional support for labels and annotations can come later, post GA.

@trentm trentm removed this from the GA milestone Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants