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

ci: remove DD unit tests metrics reporting from CI #10930

Merged
merged 1 commit into from
Feb 18, 2025
Merged

ci: remove DD unit tests metrics reporting from CI #10930

merged 1 commit into from
Feb 18, 2025

Conversation

mujahidkay
Copy link
Member

@mujahidkay mujahidkay commented Feb 3, 2025 •
edited
Loading

Apparently, our contract with DD run till September. Pending confirmation, this PR will remain draft. Integration test and benchmark results DO NOT get reported GCP right now. Ideally, we should only merge this when we handle these two but not a hard requirement IMO.

refs: https://github.com/Agoric/product-tasks/issues/236

Description

Removes all Datadog related integration for reporting unit test metrics. Also removes additional helper scripts. We are not removing integration test and benchmark test metric reporting right now - that will be done once those metrics are exported to GCP.

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

Its CI only so None

Testing Considerations

all CI.

Upgrade Considerations

None

@mujahidkay mujahidkay self-assigned this Feb 3, 2025
@cloudflare-workers-and-pages Cloudflare Workers and Pages
Copy link

cloudflare-workers-and-pages bot commented Feb 3, 2025 •
edited
Loading

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 334e0ac
Status: ✅  Deploy successful!
Preview URL: https://dde30e56.agoric-sdk.pages.dev
Branch Preview URL: https://mk-dd-removal.agoric-sdk.pages.dev

View logs

@mujahidkay mujahidkay added the force:integration Force integration tests to run on PR label Feb 3, 2025
@mujahidkay mujahidkay requested a review from Muneeb147 February 6, 2025 11:32
@Muneeb147
Copy link
Contributor

all CI. Like mentioned in the description, in its current state, we will miss out on benchmark and integration test result metrics because our existing gcp CI reporting script doesn't handle these two cases.

@mujahidkay Do have any engineering issue for it as DD sunset follow-up? Also do you think if it is doable as quick-win then we can sync and work on sending the stats first before stopping the DD reporting.
Test metrics which are currently reported to GCP can be removed from datadog.

@frazarshad Thoughts?

@mujahidkay
Copy link
Member Author

all CI. Like mentioned in the description, in its current state, we will miss out on benchmark and integration test result metrics because our existing gcp CI reporting script doesn't handle these two cases.

@mujahidkay Do have any engineering issue for it as DD sunset follow-up? Also do you think if it is doable as quick-win then we can sync and work on sending the stats first before stopping the DD reporting. Test metrics which are currently reported to GCP can be removed from datadog.

@frazarshad Thoughts?

@Muneeb147 I'm not certain how quick of a win would those be. I'm in favor of benching this PR altogether till we port those metrics to GCP (no harm done AFAIK as our contract runs till September) and hopefully we can get this done in a sprint.
I'm also not opposed to your call of removing everything but integration and benchmark reporting so let's make a call on it, and move forward

@mujahidkay mujahidkay marked this pull request as ready for review February 14, 2025 09:43
@mujahidkay mujahidkay requested a review from a team as a code owner February 14, 2025 09:43
Muneeb147
Muneeb147 reviewed Feb 14, 2025
View reviewed changes
.github/actions/post-test/action.yml
@@ -22,23 +18,6 @@ inputs:
runs:
using: composite
steps:
- name: upload tests
Copy link
Contributor

Choose a reason for hiding this comment

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

@mujahidkay Won't it stop reporting of integration and benchmarking too?

Copy link
Member Author

Choose a reason for hiding this comment

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

This step is used to collect info from individual packages' unit tests via collect-testruns.sh which further converts ava output to a junit format. And each package's junit xml is then exported to DD via

timeout 30 npx @datadog/datadog-ci junit upload --service agoric-sdk ./packages/*/junit.xml

I don't understand why this needs to be done in test-docker-build step. The info is already being sent when we run test-all-packages. On the other hand, benchmarking uses a completely different action which I have kept as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

That being said, would appreciate a second set of eyes on this. Maybe @usmanmani1122 👀

turadg
turadg reviewed Feb 14, 2025
View reviewed changes
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

To close out https://github.com/Agoric/product-tasks/issues/236 I think also requires,

  • remove patches/ava+5.3.1.patch that fed timing data to these scripts
  • remove dd-trace dep from agoric-cli
  • (possibly) remove datadog-go dep from golang/cosmos

@mujahidkay
Copy link
Member Author

mujahidkay commented Feb 17, 2025 •
edited
Loading

To close out Agoric/product-tasks#236 I think also requires,

  • remove patches/ava+5.3.1.patch that fed timing data to these scripts
  • remove dd-trace dep from agoric-cli
  • (possibly) remove datadog-go dep from golang/cosmos

You are absolutely right. I have updated the PR description to ref this issue instead of closing it, since closing it would also require replacing benchmark and integration test reporting (which we haven't planned yet). I will also update this ticket to include these deps.

@mujahidkay mujahidkay changed the title ci: remove DD metrics reporting from CI ci: remove DD unit tests metrics reporting from CI Feb 18, 2025
@mujahidkay mujahidkay added the automerge:rebase Automatically rebase updates, then merge label Feb 18, 2025
@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 18, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@mujahidkay
Copy link
Member Author

@Mergifyio requeue

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 18, 2025

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@mujahidkay mujahidkay added automerge:rebase Automatically rebase updates, then merge and removed automerge:rebase Automatically rebase updates, then merge labels Feb 18, 2025
@mergify mergify bot merged commit b962ad8 into master Feb 18, 2025
102 of 109 checks passed
@mergify mergify bot deleted the mk/dd-removal branch February 18, 2025 14:57
@mujahidkay mujahidkay mentioned this pull request Feb 19, 2025
Merged
mergify bot added a commit that referenced this pull request Feb 19, 2025
@mergify
ci: fix coverage reporting via Codecov (#11018)
0123943 
refs: #10930

## Description

#10930 unintentionally removed some codecov file generation logic which resulted in a drop of overall coverage [here](https://app.codecov.io/github/Agoric/agoric-sdk?search=&trend=7%20days). This PR attempts to fix that by reincorporating those changes. 

### Security Considerations

None

### Scaling Considerations

None

### Documentation Considerations

None

### Testing Considerations

Fixes affected Codecov report

### Upgrade Considerations

None
@mujahidkay mujahidkay mentioned this pull request Feb 20, 2025
Merged
mergify bot added a commit that referenced this pull request Feb 20, 2025
@mergify
chore: remove ava timing patch (#11024)
0e76960 
## Description

This PR removes ava timing patch which was required by a script removed in #10930. This is no longer needed.

### Security Considerations

None

### Scaling Considerations

None

### Documentation Considerations

None

### Testing Considerations

None

### Upgrade Considerations

None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turadg turadg turadg left review comments

@Muneeb147 Muneeb147 Muneeb147 approved these changes

@usmanmani1122 usmanmani1122 Awaiting requested review from usmanmani1122

Assignees

@mujahidkay mujahidkay

Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mujahidkay @Muneeb147 @turadg