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

feat: add prometheus instrumentation to checkpointlogger #159

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

matt-codecov
Copy link
Contributor

mirror CheckpointLogger data in prometheus, once this is flowing we can delete the statsd/sentry stuff

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@codecov-staging
Copy link

codecov-staging bot commented Oct 25, 2023

Codecov Report

Merging #159 (5c8b6fe) into main (db1ffbf) will decrease coverage by 0.01%.
The diff coverage is 97.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   98.39%   98.38%   -0.01%     
==========================================
  Files         348      348              
  Lines       27500    27572      +72     
==========================================
+ Hits        27058    27128      +70     
- Misses        442      444       +2     
Flag Coverage Δ
integration 98.38% <97.67%> (-0.01%) ⬇️
latest-uploader-overall 98.38% <97.67%> (-0.01%) ⬇️
unit 98.38% <97.67%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 96.89% <96.66%> (-0.01%) ⬇️
OutsideTasks 98.17% <96.29%> (-0.01%) ⬇️
Files Coverage Δ
tasks/notify.py 98.85% <100.00%> (+0.02%) ⬆️
tasks/tests/unit/test_notify_task.py 100.00% <100.00%> (ø)
helpers/checkpoint_logger/__init__.py 94.47% <94.44%> (-0.04%) ⬇️
helpers/tests/unit/test_checkpoint_logger.py 99.62% <97.22%> (-0.38%) ⬇️

@codecov-qa
Copy link

codecov-qa bot commented Oct 25, 2023

Codecov Report

Merging #159 (5c8b6fe) into main (db1ffbf) will decrease coverage by 0.01%.
The diff coverage is 97.67%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   98.39%   98.38%   -0.01%     
==========================================
  Files         348      348              
  Lines       27500    27572      +72     
==========================================
+ Hits        27058    27128      +70     
- Misses        442      444       +2     
Flag Coverage Δ
integration 98.38% <97.67%> (-0.01%) ⬇️
latest-uploader-overall 98.38% <97.67%> (-0.01%) ⬇️
unit 98.38% <97.67%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 96.89% <96.66%> (-0.01%) ⬇️
OutsideTasks 98.17% <96.29%> (-0.01%) ⬇️
Files Coverage Δ
tasks/notify.py 98.85% <100.00%> (+0.02%) ⬆️
tasks/tests/unit/test_notify_task.py 100.00% <100.00%> (ø)
helpers/checkpoint_logger/__init__.py 94.47% <94.44%> (-0.04%) ⬇️
helpers/tests/unit/test_checkpoint_logger.py 99.62% <97.22%> (-0.38%) ⬇️

@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Merging #159 (5c8b6fe) into main (db1ffbf) will decrease coverage by 0.01%.
The diff coverage is 97.67%.

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   98.35%   98.35%   -0.01%     
==========================================
  Files         374      374              
  Lines       27996    28068      +72     
==========================================
+ Hits        27535    27605      +70     
- Misses        461      463       +2     
Flag Coverage Δ
integration 98.38% <97.67%> (-0.01%) ⬇️
latest-uploader-overall 98.38% <97.67%> (-0.01%) ⬇️
unit 98.38% <97.67%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 96.79% <96.66%> (-0.01%) ⬇️
OutsideTasks 98.17% <96.29%> (-0.01%) ⬇️
Files Coverage Δ
tasks/notify.py Critical 98.85% <100.00%> (+0.02%) ⬆️
tasks/tests/unit/test_notify_task.py 100.00% <100.00%> (ø)
helpers/checkpoint_logger/__init__.py 94.53% <94.44%> (-0.05%) ⬇️
helpers/tests/unit/test_checkpoint_logger.py 99.62% <97.22%> (-0.38%) ⬇️
Related Entrypoints
run/app.tasks.notify.Notify

@codecov-public-qa
Copy link

codecov-public-qa bot commented Oct 25, 2023

Codecov Report

Merging #159 (5c8b6fe) into main (db1ffbf) will decrease coverage by 0.01%.
The diff coverage is 97.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   98.39%   98.38%   -0.01%     
==========================================
  Files         348      348              
  Lines       27500    27572      +72     
==========================================
+ Hits        27058    27128      +70     
- Misses        442      444       +2     
Flag Coverage Δ
integration 98.38% <97.67%> (-0.01%) ⬇️
latest-uploader-overall 98.38% <97.67%> (-0.01%) ⬇️
unit 98.38% <97.67%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 96.89% <96.66%> (-0.01%) ⬇️
OutsideTasks 98.17% <96.29%> (-0.01%) ⬇️
Files Coverage Δ
tasks/notify.py 98.85% <100.00%> (+0.02%) ⬆️
tasks/tests/unit/test_notify_task.py 100.00% <100.00%> (ø)
helpers/checkpoint_logger/__init__.py 94.47% <94.44%> (-0.04%) ⬇️
helpers/tests/unit/test_checkpoint_logger.py 99.62% <97.22%> (-0.38%) ⬇️

@codecov-test
Copy link

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

@matt-codecov
Copy link
Contributor Author

matt-codecov commented Oct 25, 2023

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

hmm, i was not expecting this to fire when uploading from this branch to my local codecov service. i don't think i have the PEM file it needs configured

@matt-codecov
Copy link
Contributor Author

re-requesting review because 5c8b6fe is juuuuuust significant enough that somebody should reaffirm

NotifyTask is an important part of UploadFlow but it's also used elsewhere. in those cases, NotifyTask will still log its various checkpoints despite having no data from previous checkpoints. two problems with that:

  • "Cannot compute duration" log spam
  • pollutes the reliability counters

so now NotifyTask will skip logging a checkpoint if it does not have data from previous checkpoints in the same flow. for similar reasons, made CheckpointLogger ignore repeated checkpoints instead of overwrite with new values. for the counters to be easy to analyze, each checkpoint should only occur once per flow.

@matt-codecov matt-codecov merged commit 908dc15 into main Oct 26, 2023
13 of 30 checks passed
@matt-codecov matt-codecov deleted the matt/checkpoint-logger-prometheus branch October 26, 2023 19:52
Copy link

sentry-io bot commented Nov 3, 2023

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ ValueError: Checkpoint BATCH_PROCESSING_COMPLETE not part of flow UploadFlow app.tasks.notify.Notify View Issue

Did you find this useful? React with a 👍 or 👎

@matt-codecov
Copy link
Contributor Author

expected, the serialization format of checkpoints data changed so new worker versions and old worker versions disagree

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.

4 participants