chore: introduce a pattern to help standardize log tag names #2371
Conversation
🦋 Changeset detectedLatest commit: 18306f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
apps/hubble/src/utils/logger.ts
Outdated
| return new Tags({ ...this.fields, ...tags.fields }); | ||
| } | ||
|
|
||
| build() { |
There was a problem hiding this comment.
I believe if you define toString() you won't need to call build() explicitly. This results in slightly DRYer syntax.
apps/hubble/src/utils/logger.ts
Outdated
| this.fields = obj; | ||
| } | ||
|
|
||
| addTimestamp(timestamp: number) { |
There was a problem hiding this comment.
Not clear to me from first blush that this needs to be a Farcaster timestamp. Might be worth specifying?
apps/hubble/src/utils/logger.ts
Outdated
| return new Tags({ ...this.fields, errMsg }); | ||
| } | ||
|
|
||
| addMessageFields(message: Message) { |
There was a problem hiding this comment.
Not a strong opinion, but it might make sense to implement an adapter for the logging instance that instead of passing a Tags object, it accepts an options object which knows how to handle the different types you want to annotate the tags with. For example:
log.info({ fid: ..., timestamp: ..., message: ..., error: ... });So you would define the info method like:
info(obj?: LogTags, logMsg?: string) {
const tags = new Tags(obj); // Define logic for special handling of keys
return this.log.info(tags.build(), logMsg);
}You would also allow a subset of keys to be provided, e.g.
type LogTags = {
message?: Partial<Message>; // Can provide any subset of fields for message
peerId?: string;
error?: Error;
}
...
class Tags {
constructor(obj: LogTags) {
...
// Extract `obj` into top-level keys (since `message` etc. will be deeply nested)
}
}There was a problem hiding this comment.
Hm yeah this is something I was thinking about, but I wasn't sold on it because I wasn't sure how this would evolve if we started using more functionality inside the pino logger library. Then, we'd have to expand the surface area of the adapter object and i could see that becoming a nuisance. It's also hard to remove this sort of library/object once we start using it because it will be used all over the place (we log a lot).
There was a problem hiding this comment.
I think it might make sense to use the type/constructor with all the fields on it rather than the builder pattern though (which is very verbose).
There was a problem hiding this comment.
That seems reasonable to me. Supportive!
aditiharini
force-pushed
the
standardize-log-tags
branch
from
d59303f to
006b6e7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2371 +/- ##
===========================================
- Coverage 74.05% 44.17% -29.89%
===========================================
Files 99 146 +47
Lines 9425 26280 +16855
Branches 2097 9473 +7376
===========================================
+ Hits 6980 11610 +4630
- Misses 2327 12849 +10522
- Partials 118 1821 +1703 ☔ View full report in Codecov by Sentry. |
It's hard to filter logs by tag if we name the same tag different things in different places. Introduce a pattern for constructing logs that pushes us toward standardizing log tag names.
Merge Checklist
Choose all relevant options below by adding an
xnow or at any time before submitting for reviewPR-Codex overview
This PR focuses on standardizing log tag names by introducing a
Tagsclass, which is used throughout the codebase to create consistent log entries.Detailed summary
Tagsclass inapps/hubble/src/utils/logger.tsfor standardized log tags.apps/hubble/src/hubble.ts,apps/hubble/src/network/p2p/gossipNode.ts, andapps/hubble/src/network/sync/syncHealthJob.tsto useTags.TagFieldstype definition for better structure of log fields.