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.

Sign up for GitHub

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

Jump to bottom

fix: get the logs working again #487

Merged
merged 3 commits into from
Jun 6, 2024
Merged

fix: get the logs working again #487

merged 3 commits into from
Jun 6, 2024

Conversation

joseph-sentry
Copy link
Contributor

@joseph-sentry joseph-sentry commented Jun 3, 2024 •
edited
Loading

@joseph-sentry joseph-sentry requested a review from a team June 3, 2024 20:33
@codecov-staging
Copy link

codecov-staging bot commented Jun 3, 2024 •
edited by codecov-notifications bot
Loading

Codecov Report

Attention: Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
- Coverage   97.26%   97.26%   -0.01%     
==========================================
  Files         412      412              
  Lines       34393    34411      +18     
==========================================
+ Hits        33453    33470      +17     
- Misses        940      941       +1
Flag Coverage Δ
integration 97.26% <96.00%> (-0.01%) ⬇️
latest-uploader-overall 97.26% <96.00%> (-0.01%) ⬇️
unit 97.26% <96.00%> (-0.01%) ⬇️

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

Components Coverage Δ
NonTestCode 94.42% <95.65%> (+<0.01%) ⬆️
OutsideTasks 97.54% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
helpers/logging_config.py 83.63% <100.00%> (+2.78%) ⬆️
helpers/tests/unit/test_logging_config.py 100.00% <100.00%> (ø)
tasks/base.py 95.69% <90.00%> (-0.33%) ⬇️

@codecov-qa
Copy link

codecov-qa bot commented Jun 3, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.26%. Comparing base (49e8df9) to head (1fdb4ed).

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
- Coverage   97.26%   97.26%   -0.01%     
==========================================
  Files         412      412              
  Lines       34393    34411      +18     
==========================================
+ Hits        33453    33470      +17     
- Misses        940      941       +1
Flag Coverage Δ
integration 97.26% <96.00%> (-0.01%) ⬇️
latest-uploader-overall 97.26% <96.00%> (-0.01%) ⬇️
unit 97.26% <96.00%> (-0.01%) ⬇️

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

Components Coverage Δ
NonTestCode 94.42% <95.65%> (+<0.01%) ⬆️
OutsideTasks 97.54% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
helpers/logging_config.py 83.63% <100.00%> (+2.78%) ⬆️
helpers/tests/unit/test_logging_config.py 100.00% <100.00%> (ø)
tasks/base.py 95.69% <90.00%> (-0.33%) ⬇️

@codecov-public-qa Codecov Public QA
Copy link

codecov-public-qa bot commented Jun 3, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 97.26%. Comparing base (49e8df9) to head (1fdb4ed).

✅ All tests successful. No failed tests found ☺️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
- Coverage   97.26%   97.26%   -0.01%     
==========================================
  Files         412      412              
  Lines       34393    34411      +18     
==========================================
+ Hits        33453    33470      +17     
- Misses        940      941       +1
Flag Coverage Δ
integration 97.26% <96.00%> (-0.01%) ⬇️
latest-uploader-overall 97.26% <96.00%> (-0.01%) ⬇️
unit 97.26% <96.00%> (-0.01%) ⬇️

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

Components Coverage Δ
NonTestCode 94.42% <95.65%> (+<0.01%) ⬆️
OutsideTasks 97.54% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
helpers/logging_config.py 83.63% <100.00%> (+2.78%) ⬆️
helpers/tests/unit/test_logging_config.py 100.00% <100.00%> (ø)
tasks/base.py 95.69% <90.00%> (-0.33%) ⬇️

@codecov Codecov
Copy link

codecov bot commented Jun 3, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.29%. Comparing base (49e8df9) to head (1fdb4ed).

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

✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
- Coverage   97.29%   97.29%   -0.01%     
==========================================
  Files         443      443              
  Lines       35122    35140      +18     
==========================================
+ Hits        34172    34189      +17     
- Misses        950      951       +1
Flag Coverage Δ
integration 97.26% <96.00%> (-0.01%) ⬇️
latest-uploader-overall 97.26% <96.00%> (-0.01%) ⬇️
unit 97.26% <96.00%> (-0.01%) ⬇️

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

Components Coverage Δ
NonTestCode 94.47% <95.65%> (-0.01%) ⬇️
OutsideTasks 97.54% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
helpers/logging_config.py Critical 96.36% <100.00%> (+0.61%) ⬆️
helpers/tests/unit/test_logging_config.py 100.00% <100.00%> (ø)
tasks/base.py Critical 95.69% <90.00%> (-0.33%) ⬇️
Related Entrypoints
run/app.tasks.bundle_analysis.BundleAnalysisProcessor
run/app.tasks.upload.Upload
run/app.tasks.status.SetError
run/app.tasks.notify.Notify
run/app.tasks.timeseries.save_commit_measurements
run/app.tasks.profiling.normalizer
run/app.tasks.pulls.Sync
run/app.tasks.compute_comparison.ComputeComparison
run/app.tasks.upload.UploadFinisher
run/app.tasks.upload.UploadProcessor
run/app.tasks.commit_update.CommitUpdate
run/app.tasks.upload.PreProcessUpload
run/app.tasks.profiling.collection
run/app.tasks.upload.ParallelVerification
run/app.tasks.sync_repo_languages_gql.SyncLanguagesGQL
run/app.tasks.bundle_analysis.BundleAnalysisNotify
run/app.tasks.test_results.TestResultsFinisherTask
run/app.tasks.sync_repo_languages.SyncLanguages
run/app.cron.profiling.findinguncollected
run/app.tasks.profiling.summarization
run/app.tasks.static_analysis.check_suite
run/app.tasks.sync_repos.SyncRepos
run/app.cron.hourly_check.HourlyCheckTask
run/app.tasks.test_results.TestResultsProcessor
run/app.tasks.new_user_activated.NewUserActivated
run/app.tasks.sync_teams.SyncTeams
run/app.tasks.timeseries.backfill_commits
run/app.cron.plan.TrialExpirationCronTask
run/app.cron.daily.GitHubAppWebhooksCheckTask
run/app.tasks.timeseries.backfill_dataset
run/app.tasks.label_analysis.process_request
run/app.tasks.flush_repo.FlushRepo
run/app.tasks.upload.ManualUploadCompletionTrigger
run/app.cron.daily.BrollyStatsRollupTask

giovanni-guidini
giovanni-guidini reviewed Jun 4, 2024
View reviewed changes
Copy link
Contributor

@giovanni-guidini giovanni-guidini left a comment

Choose a reason for hiding this comment

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

This hack assumes that only 1 task runs at any given time in a single process. I think it's a fair assumption, but have you verified it? Otherwise we can have logs with wrong task_ids

helpers/logging_config.py
task_name = None
task_id = None

# this is a temporary hack to get the task information working in the logs again
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless there's a GitHub ticket to fix this for good there are good chances that this "temporary hack" will become permanent

@joseph-sentry joseph-sentry requested review from a team and removed request for a team June 4, 2024 16:58
@joseph-sentry
Copy link
Contributor Author

This hack assumes that only 1 task runs at any given time in a single process. I think it's a fair assumption, but have you verified it? Otherwise we can have logs with wrong task_ids

I agree if there are multiple tasks running at once in a given process. this will be an issue. I added a log line to check if there is concurrent access and i'm going to run this on staging for a while to verify this.

@joseph-sentry
Copy link
Contributor Author

https://l.codecov.dev/deei0v

logs here look good to me

@joseph-sentry
fix: get the logs working again
290de75 
Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
@joseph-sentry
fix: clean up global logs vars after task runs
11974f3 
Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
@joseph-sentry
fix: add warning log if concurrent access to log vars
1fdb4ed 
Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
@joseph-sentry joseph-sentry added this pull request to the merge queue Jun 6, 2024
Merged via the queue into main with commit d0a3b6a Jun 6, 2024
36 of 43 checks passed
@joseph-sentry joseph-sentry deleted the joseph/fix-logs branch June 6, 2024 17:17
adrian-codecov pushed a commit that referenced this pull request Jun 8, 2024
@joseph-sentry @adrian-codecov
fix: get the logs working again (#487)
52f8242 
Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giovanni-guidini giovanni-guidini giovanni-guidini approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate and repair why torngit and notify code does not log task id information
2 participants
@joseph-sentry @giovanni-guidini