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

Clarify activation method when version unknown #783

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

SylvainJuge
Copy link
Member

@SylvainJuge SylvainJuge commented Mar 31, 2023

Assume that the server version is not 8.7.0 and send the activation_method by default.

  • Create PR as draft
  • Approval by at least one other agent
  • Mark as Ready for Review (automatically requests reviews from all agents and PM via CODEOWNERS)
    • Remove PM from reviewers if impact on product is negligible
    • Remove agents from reviewers if the change is not relevant for them
  • Approved by at least 2 agents + PM (if relevant)
  • Merge after 7 days passed without objections
    To auto-merge the PR, add /schedule YYYY-MM-DD to the PR description.
  • Create implementation issues through the meta issue template (this will automate issue creation for individual agents)
  • If this spec adds a new dynamic config option, add it to central config.

@apmmachine
Copy link

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-03-31T08:15:59.810+0000

  • Duration: 3 min 7 sec

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I much prefer this optimistic default to send it UNLESS we explicitly know its the version with the bug not the other way around.

specs/agents/metadata.md Outdated Show resolved Hide resolved
@trentm
Copy link
Member

trentm commented Mar 31, 2023

Actually, wait. This wording of the spec suggests that we only want to send activation_method for >=8.7.1.
Do we want activation_method data for earlier APM server versions, like 8.5, 8.6? Or is it not useful for those earlier versions?

@basepi
Copy link
Contributor

basepi commented Mar 31, 2023

Yeah, worth noting that we're doing this for >=8.7.1, no lower versions. And this spec change matches our behavior already. elastic/apm-agent-python#1787

Co-authored-by: Trent Mick <trentm@gmail.com>
@SylvainJuge SylvainJuge self-assigned this Apr 3, 2023
@SylvainJuge SylvainJuge marked this pull request as ready for review April 3, 2023 07:57
@SylvainJuge SylvainJuge requested review from a team as code owners April 3, 2023 07:57
@SylvainJuge SylvainJuge removed request for a team April 3, 2023 07:57
@Mpdreamz
Copy link
Member

Mpdreamz commented Apr 4, 2023

I just found that in .NET we basically only do the get server information API call once.

If a user upgrades, we'll continue to not send activation method untill the users app restarts.

To me this is an acceptable tradeoff vs having no metrics. Wonder how everyone else feels about this though.

@SylvainJuge
Copy link
Member Author

I just found that in .NET we basically only do the get server information API call once.

In Java too, we only do it once on agent startup, and we also have to make that call asynchronous to prevent blocking when trying to get the agent remote configuration. Also, we only serialize metadata once which further complicates the logic here.

If a user upgrades, we'll continue to not send activation method untill the users app restarts.

You mean upgrade the apm-server, not the apm agent here ? For Java (and likely others), this effectively means that if the agent is started with a 8.7.0 server, then it won't send it until the application is restarted.

To me this is an acceptable tradeoff vs having no metrics. Wonder how everyone else feels about this though.

💯 here, however I'm pretty sure it's a topic that will come back with serverless and managed apm server instance.
One possible approach would be to make apm-server provide the effective version through HTTP response headers, so the agent can adapt to the apm-server features dynamically when sending data (or fetching remote configuration).

@beniwohli
Copy link
Contributor

Same in Python, we only get the server version at agent initialization. Maybe it would make sense to add the server version to e.g. the config endpoint response at some point, but I agree that the current behaviour is fine for now.

Mpdreamz added a commit to elastic/apm-agent-dotnet that referenced this pull request Apr 4, 2023
Implements spec change from elastic/apm#783

Also normalizes Prerelease in ElasticVersion to always be string.Empty. The
current Equals implementation does not handle null<=>string.Empty well.
@trentm
Copy link
Member

trentm commented Apr 4, 2023

I just found that in .NET we basically only do the get server information API call once.

Same for the Node.js agent.

Mpdreamz added a commit to elastic/apm-agent-dotnet that referenced this pull request Apr 5, 2023
* Prevent sending activation_method in metadata for 8.7.0

Implements spec change from elastic/apm#783

Also normalizes Prerelease in ElasticVersion to always be string.Empty. The
current Equals implementation does not handle null<=>string.Empty well.

* update xmldoc

* Apply suggestions from code review

Co-authored-by: Gergely Kalapos <gergo@kalapos.net>

* Format and remove unused usings

* fix build error

---------

Co-authored-by: Gergely Kalapos <gergo@kalapos.net>
@SylvainJuge
Copy link
Member Author

I will merge as we all agree on this one, also it seems that the implementation in agents also fit this already, so I won't create a meta+agent implementation issues here.

Feel free to create issues and PRs linked to this one if needed though.

@SylvainJuge SylvainJuge merged commit 6b807a5 into elastic:main Apr 13, 2023
@SylvainJuge SylvainJuge deleted the activation-method-take2 branch April 13, 2023 08:01
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

Successfully merging this pull request may close these issues.

9 participants