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.

Sign up for GitHub

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

Jump to bottom

feat(#8216): propagate api request id to haproxy #9613

Merged
merged 12 commits into from
Nov 7, 2024
Merged

feat(#8216): propagate api request id to haproxy #9613

merged 12 commits into from
Nov 7, 2024

Conversation

dianabarsan
Copy link
Member

@dianabarsan dianabarsan commented Nov 5, 2024 •
edited
Loading

Description

Adds custom header to pass the id of the original client request from API to all cascading requests to CouchDb.
Changes request ID to being a 12 char long string (uuid slice) instead of the whole uuid.

#8216

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

Compose URLs

If Build CI hasn't passed, these may 404:

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@dianabarsan
adds async local storage in API
456bade 
Signed-off-by: Diana Barsan <[email protected]>
@dianabarsan
adds e2e test
f8618c2 
Signed-off-by: Diana Barsan <[email protected]>
@dianabarsan
adds e2e test
c7d4498 
Signed-off-by: Diana Barsan <[email protected]>
@dianabarsan
make test idempotent. don't reuse username
266f6c3 
Signed-off-by: Diana Barsan <[email protected]>
garethbowen
garethbowen reviewed Nov 5, 2024
View reviewed changes
api/src/routing.js Outdated Show resolved Hide resolved
@dianabarsan
Copy link
Member Author

Since you've already been here, @garethbowen , mind a proper review? Thanks!

garethbowen
garethbowen requested changes Nov 5, 2024
View reviewed changes
Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

TIL! That's cool. Java thread local flashbacks...

Anyway, some suggestions inline.

api/src/db.js Show resolved Hide resolved
tests/integration/api/server.spec.js Outdated Show resolved Hide resolved
tests/integration/api/server.spec.js Show resolved Hide resolved
tests/integration/api/server.spec.js Outdated Show resolved Hide resolved
tests/integration/api/server.spec.js Outdated Show resolved Hide resolved
tests/integration/api/server.spec.js Show resolved Hide resolved
api/src/routing.js Show resolved Hide resolved
api/src/services/async-storage.js Outdated Show resolved Hide resolved
garethbowen
garethbowen reviewed Nov 6, 2024
View reviewed changes
api/src/routing.js Outdated Show resolved Hide resolved
@garethbowen
Copy link
Member

It would also be worth checking that whatever we land on here works with 3rd party tools to pull logs from multiple services, eg: https://grafana.com/docs/grafana/latest/explore/simplified-exploration/logs/ (released like a month ago!)

@dianabarsan
Copy link
Member Author

It would also be worth checking that whatever we land on here works with 3rd party tools to pull logs from multiple services, eg: https://grafana.com/docs/grafana/latest/explore/simplified-exploration/logs/ (released like a month ago!)

I think we already have a custom logging structure, we don't follow some specific standard.

@dianabarsan dianabarsan changed the title feat(#8216): propagate api request uuid to haproxy feat(#8216): propagate api request id to haproxy Nov 6, 2024
@garethbowen
Copy link
Member

I think we already have a custom logging structure, we don't follow some specific standard.

I agree. IMO the killer feature is for our observability server to be able to show all things that happened across all services for a specific request. I'm assuming this change will make that work, but if you felt like testing loki to make sure that'd be valuable. It should work with custom logging structures.

garethbowen
garethbowen approved these changes Nov 7, 2024
View reviewed changes
Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Awesome! Nice one.

tests/integration/api/server.spec.js
expect(haproxyHydrateReqs.length).to.equal(2);

expect(hydrateReqId).not.to.equal(configReqId);
});
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@dianabarsan dianabarsan merged commit d2bebe8 into master Nov 7, 2024
48 checks passed
@dianabarsan dianabarsan deleted the 8216-propagate-req-uuid-to-haproxy branch November 7, 2024 20:57
sugat009 pushed a commit that referenced this pull request Nov 18, 2024
@dianabarsan @sugat009
feat(#8216): propagate api request id to haproxy (#9613)
387d4ed 
Adds custom header to pass the id of the original client request from API to all cascading requests to CouchDb.
Changes request ID to being a 12 char long string (uuid slice) instead of the whole uuid.

#8216
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garethbowen garethbowen garethbowen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dianabarsan @garethbowen