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

minor log fix #485

Merged
merged 3 commits into from
Feb 27, 2024
Merged

minor log fix #485

merged 3 commits into from
Feb 27, 2024

Conversation

denis-yuen
Copy link
Member

@denis-yuen denis-yuen commented Feb 21, 2024

Description
Iterate version and minor fix to logging (do not increment count if submissions failed)

Review Instructions
When ingesting new metrics, should see more accurate count. May need to wait for more data

Issue
https://ucsc-cgl.atlassian.net/browse/SEAB-6218

Security
If there are any concerns that require extra attention from the security team, highlight them here.

Please make sure that you've checked the following before submitting your pull request. Thanks!

  • Check that you pass the basic style checks and unit tests by running mvn clean install in the project that you have modified (until https://ucsc-cgl.atlassian.net/browse/SEAB-5300 adds multi-module support properly)
  • Ensure that the PR targets the correct branch. Check the milestone or fix version of the ticket.
  • If you are changing dependencies, check with dependabot to ensure you are not introducing new high/critical vulnerabilities
  • If this PR is for a user-facing feature, create and link a documentation ticket for this feature (usually in the same milestone as the linked issue). Style points if you create a documentation PR directly and link that instead.

@denis-yuen denis-yuen self-assigned this Feb 21, 2024
@denis-yuen denis-yuen changed the title Feature/minor log fix minor log fix Feb 21, 2024
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

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

Project coverage is 57.10%. Comparing base (da1c97f) to head (abc108f).

Files Patch % Lines
...csaggregator/client/cli/TerraMetricsSubmitter.java 50.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop     #485      +/-   ##
=============================================
- Coverage      57.13%   57.10%   -0.03%     
  Complexity       304      304              
=============================================
  Files             32       32              
  Lines           1990     1991       +1     
  Branches         167      167              
=============================================
  Hits            1137     1137              
- Misses           779      780       +1     
  Partials          74       74              
Flag Coverage Δ
metricsaggregator 49.27% <50.00%> (-0.03%) ⬇️
toolbackup 39.07% <0.00%> (-0.02%) ⬇️
tooltester 31.24% <0.00%> (-0.02%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@denis-yuen denis-yuen marked this pull request as ready for review February 21, 2024 21:51
@@ -199,7 +199,6 @@ private void executionMetricsPost(List<RunExecution> workflowExecutionsToSubmit,
try {
extendedGa4GhApi.executionMetricsPost(new ExecutionsRequestBody().runExecutions(workflowExecutionsToSubmit), Partner.TERRA.toString(), sourceUrlTrsInfo.trsId(),
sourceUrlTrsInfo.version(), description);
numberOfExecutionsSubmitted.addAndGet(workflowMetricRecords.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I would think this location would also only count it if there's no exception. I believe the problem is that we're logging the count of workflowMetricRecords which may not be the same as the workflow executions that are being submitted via workflowExecutionsToSubmit.

I think we should add the count of workflowExecutionsToSubmit instead of workflowMetricRecords because the former gets partitioned and contains the accurate count

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, totally missed this and makes sense

@denis-yuen denis-yuen requested review from kathy-t and removed request for kathy-t February 26, 2024 15:21
Copy link

sonarcloud bot commented Feb 26, 2024

@denis-yuen denis-yuen requested review from a team, david4096, ll5zh and coverbeck and removed request for a team February 27, 2024 15:34
@denis-yuen denis-yuen merged commit dc5b9af into develop Feb 27, 2024
9 of 10 checks passed
@denis-yuen denis-yuen deleted the feature/minor_log_fix branch February 27, 2024 21:02
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.

3 participants