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

Release/3.2.1 #435

Merged
merged 14 commits into from
Nov 25, 2024
Merged

Release/3.2.1 #435

merged 14 commits into from
Nov 25, 2024

Conversation

peel
Copy link
Contributor

@peel peel commented Nov 22, 2024

Jira ref: PDP-1158

peel and others added 13 commits April 10, 2024 09:22
Previously `Content-Type` was not explicitly returned for POST requests.
This would result in errors being logged for javascript tracker in Firefox[1].
Now, the behavior is rolled back to previous where collector would return
`Content-Type` header for these requests.

Addresses [BCPF-1102] and [PDP-1110]

---

1 - https://bugzilla.mozilla.org/show_bug.cgi?id=884693
The goal of the feature was to prevent long body reads in GCP, this however does
not prevent the slow incoming connection handling at the framework level.
Therefore, as this adds unnecessary complexity with possible negative
performance impact, the feature is removed.

The configuration parameter is no longer used, but can remain as is.
Currently, healthchecks reside behind the same timeout settings as any other
endpoint. We observed that when autoscaling under massive load, it is possible
for collector to be taken down because of long health check responses.
Which previously did not happen. We therefore move healthchecks above the
timeout middleware to return to previous behavior.
Additionally, this allows us to set arbitrarily short (or long) response times
for the regular endpoints when necessary.

---

Part of [PDP-1408].
The reference defaults should be less strict and match the settings we define
upstream.
Previously, we would return 503 Service Unavailable, suggesting that failures
should not be retried and leading to confusion with early timeout being hit.
Now, we return 408 Request Timeout which is more explicit and easier to monitor.
@peel peel force-pushed the release/3.2.1 branch 4 times, most recently from 703383b to f066c37 Compare November 25, 2024 13:01
@peel peel marked this pull request as ready for review November 25, 2024 13:27
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it have this [Unreleased] bit? Is there an automation that fills it out or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to keepachangelog the header should stay there: https://keepachangelog.com/en/1.1.0/#effort
For now there is no automation. We can get it done separately. The goal for now is to keep changes in readable sections so it's easy to find functional the differences between versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough - I don't really like it and would argue that it's just noise on top of the Changelog, which doesn't seem to serve the purpose stated in that doc, because we never merge unreleased changes. IMO the same thing can be achieved by making a new section when we make a new develop branch.

But that's probably just a personal preference, I'm not bothered and wouldn't fight you on it. Certainly wouldn't block a release because of it! Just an opinion for consideration :)

Copy link
Contributor

@colmsnowplow colmsnowplow 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 did review the code but ofc not the best placed to spot issues, assuming we are confident in our previous reviews of the implementation.

@peel peel merged commit 5e47261 into master Nov 25, 2024
3 checks passed
@peel peel deleted the release/3.2.1 branch November 25, 2024 14:38
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.

6 participants