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 Correlation ID support to client error + activity tracking records #338

Merged
merged 10 commits into from
Aug 7, 2024

Conversation

ghsolomon
Copy link
Contributor

…tivity tracking

ghsolomon added 4 commits July 3, 2024 13:41
# Conflicts:
#	CHANGELOG.md
#	grails-app/controllers/io/xh/hoist/impl/XhController.groovy
#	grails-app/domain/io/xh/hoist/clienterror/ClientError.groovy
#	grails-app/domain/io/xh/hoist/track/TrackLog.groovy
#	grails-app/services/io/xh/hoist/clienterror/ClientErrorService.groovy
Copy link
Member

@lbwexler lbwexler left a comment

Choose a reason for hiding this comment

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

👍🏻

CHANGELOG.md Outdated
### 🎁 New Features

* Added support for tracking Correlation ID's in Activity and Client Error logs.
Requires `hoist-react >= 67.0.0`.
Copy link
Member

Choose a reason for hiding this comment

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

I assume that we mean the feature requires hoist-react, not the overall hoist-core version - i.e. you can still take this core update even if you are not ready to take the react update. We should clarify, should be examples in this CL of that language

Copy link
Member

Choose a reason for hiding this comment

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

We should also include example SQL to add the columns - see other examples in this file

@@ -61,7 +61,7 @@ class ClientErrorService extends BaseService {
* @param appVersion - expected from client to ensure we record the version user's browser is actually running
* @param url - location where error occurred
Copy link
Member

Choose a reason for hiding this comment

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

doc string getting (more) out of date

@@ -68,6 +70,7 @@ class TrackLog implements JSONFormat {
device: device,
userAgent: userAgent,
category: category,
correlationId: correlationId,
Copy link
Member

Choose a reason for hiding this comment

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

Tiny thing, but let's move this up after ID, as with ClientError. I always want some kind of recipe for more consistency here, don't think we really have one, but might as well add the new prop in the same place.

@amcclain amcclain changed the title Add support for Correlation ID's across fetch requests and error / ac… Add Correlation ID support to client error + activity tracking records Jul 30, 2024
@amcclain
Copy link
Member

Noting for the record that we've discussed some other ways hoist-core could incorporate correlation IDs:

  • Mixing them into default logging formats
  • Adding support to HttpClient to auto-generate / pass through

BUT - I think it is prudent to get a first cut out with the basic support we have here + the support in hoist-react, then see how they start to percolate through app code in practice and revisit these additional hoist-core features in the future.

@lbwexler lbwexler merged commit 0d4f3bc into develop Aug 7, 2024
4 checks passed
@lbwexler lbwexler deleted the correlationIDSupport branch August 7, 2024 00:40
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.

3 participants