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

Trace Configuration Trace Message Enhancement #5480

Merged
merged 3 commits into from
Sep 25, 2023

Conversation

jutaro
Copy link
Contributor

@jutaro jutaro commented Sep 21, 2023

This pull request aims to enhance our codebase by adding trace messages that provide valuable insights into the effective trace configuration used during program execution. Trace messages are essential for debugging and monitoring our application, and this enhancement will make it easier for developers and operators to understand and troubleshoot any issues related to tracing.

@jutaro jutaro self-assigned this Sep 21, 2023
@jutaro jutaro requested a review from mgmeier September 21, 2023 07:45
@jutaro jutaro marked this pull request as ready for review September 21, 2023 09:01
@jutaro jutaro requested review from a team as code owners September 21, 2023 09:01
}
deriving (Eq, Ord, Show, Generic)

instance AE.FromJSON ConfigOptionRep where
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why deriving (FromJSON, ToJSON) isn't used? I can't really spot a difference in the handcrafted instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The maybe fields are only printed, when the value is not Nothing. This is hand coded, so that I used handcrafted instances here. I think this is only possible with Template Haskell automatically, but in my opinion the handwritten stuff is preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, don't use TH for that; Generics is preferrable. With the latter, you can use:

instance ToJSON ConfigRepresentation where
    toJSON = genericToJSON defaultOptions {omitNothingFields = True}

, "traceOptionNodeName" .= traceOptionNodeName
, "TraceOptionPeerFrequency" .= traceOptionPeerFrequency
, "traceOptionResourceFrequency" .= traceOptionResourceFrequency
]
Copy link
Contributor

@mgmeier mgmeier Sep 21, 2023

Choose a reason for hiding this comment

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

The capitalization of object keys doesn't align between FromJSON and ToJSON, is that intentional?
If there's a Generic instance for everything (not sure), one could use derived instances, or for capital initials something like:

instance FromJSON ConfigRepresentation where
    parseJSON = genericParseJSON defaultOptions {fieldLabelModifier =  <capitalize first letter>}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected the capitalisation. Thanks for pointing this out. It is only one unused Generic instance here, which I remove.

trace-dispatcher/src/Cardano/Logging/Utils.hs Outdated Show resolved Hide resolved
@jutaro jutaro force-pushed the jutaro/new-tracing-open-ends branch 3 times, most recently from 287d2bd to 71200e5 Compare September 22, 2023 10:05
Copy link
Contributor

@mgmeier mgmeier left a comment

Choose a reason for hiding this comment

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

Thanks @jutaro LGTM!

@mgmeier mgmeier added this pull request to the merge queue Sep 25, 2023
Merged via the queue into master with commit 7b1c60e Sep 25, 2023
24 checks passed
@mgmeier mgmeier deleted the jutaro/new-tracing-open-ends branch September 25, 2023 20:50
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.

None yet

3 participants