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

Add APM agent Breaking Changes doc #650

Merged
merged 4 commits into from
Jul 4, 2022

Conversation

estolfo
Copy link
Contributor

@estolfo estolfo commented Jun 14, 2022

I've written what we consider to be a breaking change in an APM agent in this document.
It can act as a guide as we improve our agents, add new features, change our Public API, etc.
I think I've covered all the scenarios and nuances but I may have missed some important points. So please review, comment, and add to the document as you see fit.

@estolfo estolfo requested review from a team as code owners June 14, 2022 10:54
@apmmachine
Copy link

apmmachine commented Jun 14, 2022

💚 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: 2022-07-04T05:38:00.685+0000

  • Duration: 5 min 18 sec

Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

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

Looks good. Once we have more information from the clients team and the breaking changes committee we should add more implementation details here. (See today's agents weekly meeting for the discussion around this)

Co-authored-by: Trent Mick <trentm@gmail.com>
@jaggederest
Copy link
Contributor

I think we might also want to expand on the idea that we consider significant changes in security or privacy surface area or posture (e.g. sanitization config) that might expose more data to be breaking. I'm not sure if that's the case, but when I'm acting as a library user I usually want those kind of things to be called out in a major version.

Might also want to mention the Semver standards if we're broadly sticking with those in the agents, though that could go in the subsidiary implementation details per @basepi above 👍

@estolfo
Copy link
Contributor Author

estolfo commented Jun 16, 2022

I think we might also want to expand on the idea that we consider significant changes in security or privacy surface area or posture (e.g. sanitization config) that might expose more data to be breaking. I'm not sure if that's the case, but when I'm acting as a library user I usually want those kind of things to be called out in a major version.

Thanks for the suggestion, @jaggederest. Do you have an example in mind of where we might make this kind of change? You can also make a suggestion to the PR with more text, if you'd like.

Regarding Semver standards, I think we'll move that subject to the how discussion. But definitely an important subject worth discussing and documenting.

@jaggederest
Copy link
Contributor

Just as an example, if we changed to send usage or registration data to elastic servers, for example, for debugging or survey purposes, I'd expect that to be considered a Big Deal. For security side, if we changed our sanitizers to be less comprehensive (might increase PII in the logs / APM server). I don't expect we would want to make those kinds of change any time soon but that's an example of what I mean.

@basepi
Copy link
Contributor

basepi commented Jun 20, 2022

The Python agent has only released one breaking change release in almost 3 years. 5.0.0 was in August 2019, 6.0.0 was in February 2021. It's now June 2022. We're pretty good about not making breaking changes overall, so the make-it-minor policy will probably be fairly low-impact.

However, one big thing that concerns me is how to nicely inform the user of coming breaking changes.

Take, for example, this breaking change from 6.0.0:

Remove HTTP querystring sanitization. This improves performance, and is meant to standardize behavior across the
agents, as defined in #334.

Let's say we want to signal this change 18 months ahead of implementation. Is adding an "upcoming breaking changes" doc section acceptable? Do we log at the user every time we see an HTTP querystring? Or log at startup for everyone for 18 months? (I, as a user, would consider that kind of spam logging to be a bug.) This is why the implementation details interest me so much.

Because breaking changes happen so infrequently in the Python agent, we expect users to notice that we released a major and look at the release notes where we have a prominent Breaking Changes section. (That's how we've done it historically, anyway). Obviously that's not in line with make-it-minor, but I'm very invested in making our notification solution not suck for the user.

@estolfo
Copy link
Contributor Author

estolfo commented Jun 21, 2022

Thanks for your input, @basepi. After we get this document finalized, we'll start another effort to define how we introduce breaking changes. Notifying the user of upcoming breaking changes and of those changes when they are made will definitely be part of our discussion.

Co-authored-by: Justin George <jaggederest@users.noreply.github.com>
@estolfo estolfo merged commit bc89953 into elastic:main Jul 4, 2022
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.

8 participants