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

log: debug manifest cache #9602

Merged
merged 4 commits into from Jun 16, 2022

Conversation

crenshaw-dev
Copy link
Collaborator

Some more logging. It was critical to debugging #9601, and I think it might be useful in the future.

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

fix silly refactor

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #9602 (b468beb) into master (50bf41f) will decrease coverage by 0.03%.
The diff coverage is 57.81%.

@@            Coverage Diff             @@
##           master    #9602      +/-   ##
==========================================
- Coverage   45.82%   45.78%   -0.04%     
==========================================
  Files         222      226       +4     
  Lines       26376    26713     +337     
==========================================
+ Hits        12087    12231     +144     
- Misses      12642    12818     +176     
- Partials     1647     1664      +17     
Impacted Files Coverage Δ
util/webhook/webhook.go 67.66% <0.00%> (-0.69%) ⬇️
...licationset/generators/generator_spec_processor.go 51.66% <46.42%> (-5.91%) ⬇️
reposerver/cache/cache.go 60.52% <47.36%> (-3.76%) ⬇️
...cationset/controllers/applicationset_controller.go 56.75% <100.00%> (ø)
applicationset/generators/matrix.go 72.60% <100.00%> (+0.38%) ⬆️
applicationset/generators/merge.go 60.60% <100.00%> (+0.40%) ⬆️
applicationset/utils/utils.go 78.40% <100.00%> (ø)
reposerver/repository/repository.go 61.91% <100.00%> (+0.52%) ⬆️
server/application/websocket.go 8.33% <0.00%> (-4.57%) ⬇️
server/application/logs.go 82.45% <0.00%> (-3.26%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 001c7b2...b468beb. Read the comment docs.

@crenshaw-dev crenshaw-dev requested a review from leoluz June 8, 2022 18:02
Copy link
Collaborator

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

Function signatures are already huge and we can avoid adding additional parameters to them if we log in the caller function.

reposerver/cache/cache.go Outdated Show resolved Hide resolved
reposerver/cache/cache.go Outdated Show resolved Hide resolved
reposerver/cache/cache.go Outdated Show resolved Hide resolved
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
// LogDebugManifestCacheKeyFields logs all the information included in a manifest cache key. It's intended to be run
// before every manifest cache operation to help debug cache misses.
func LogDebugManifestCacheKeyFields(message string, reason string, revision string, appSrc *appv1.ApplicationSource, clusterInfo ClusterRuntimeInfo, namespace string, trackingMethod string, appLabelKey string, appName string) {
if log.IsLevelEnabled(log.DebugLevel) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this check is required.
Logrus already checks for this inside the Debug function:
https://github.com/sirupsen/logrus/blob/v1.8.1/logger.go#L195-L201

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just a small performance enhancement. Calculating the fields involves some JSON marshaling that I'd rather avoid unless we know we're going to log.

Copy link
Collaborator

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

LGTM

@leoluz leoluz merged commit c478617 into argoproj:master Jun 16, 2022
@crenshaw-dev crenshaw-dev deleted the debug-manifest-cache-master branch June 16, 2022 15:36
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

2 participants