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

🐛 [RUM-3599] do not define undefined instrumented method #2662

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

BenoitZugmeyer
Copy link
Member

Motivation

Some websites are removing native methods on purpose. Re-defining the method breaks them.

Changes

Don't define undefined instrumented methods.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner March 20, 2024 10:10
@BenoitZugmeyer
Copy link
Member Author

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Mar 20, 2024

🚂 Branch Integration: starting soon, merge in < 10m

Commit e1b6ea5c12 will soon be integrated into staging-12.

This build is going to start soon! (estimated merge in less than 10m)

Use /to-staging -c to cancel this operation!

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Mar 20, 2024

🚨 Branch Integration: The build pipeline contains failing jobs for this merge request

We couldn't automatically merge the commit e1b6ea5c12 into staging-12.
Build pipeline has failing jobs for eaf425d

Since those jobs are not marked as being allowed to fail, the pipeline will most likely fail.
Therefore, and to allow other builds to be processed, this merge request has been rejected before the end of the pipeline.

You should have a look at the pipeline, wait for the build to finish and investigate the failures.
When you are ready, re-add your pull request to the queue!

⚠️ Do NOT retry failed jobs directly.

They were executed on a temporary branch created by the merge queue. If you retry them, they are going to fail because the branch no longer exists.

If you think the errors come from a logical conflict with the target branch, you can create a fix by commenting this pull request with /create-fix-branch -b staging-12

Details

List of failed jobs:

Go to failed Gitlab pipeline.

If you need support, contact us on Slack #ci-interfaces with those details!

@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/fix-undefined-instrument-method branch from e1b6ea5 to fa634d6 Compare March 20, 2024 10:40
@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.90%. Comparing base (c52216b) to head (00103f1).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2662      +/-   ##
==========================================
- Coverage   92.91%   92.90%   -0.02%     
==========================================
  Files         239      239              
  Lines        6931     6934       +3     
  Branches     1517     1518       +1     
==========================================
+ Hits         6440     6442       +2     
- Misses        491      492       +1     

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

Copy link

cit-pr-commenter bot commented Mar 20, 2024

Bundles Sizes Evolution

📦 Bundle Name Base Size Local Size 𝚫% Status
Rum 155.94 kB 155.98 kB +0.03%
Logs 55.15 kB 55.19 kB +0.07%
Rum Slim 101.53 kB 101.57 kB +0.04%
Worker 25.21 kB 25.21 kB 0.00%

@BenoitZugmeyer
Copy link
Member Author

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Mar 20, 2024

🚂 Branch Integration: starting soon, merge in < 10m

Commit fa634d6162 will soon be integrated into staging-12.

This build is going to start soon! (estimated merge in less than 10m)

Use /to-staging -c to cancel this operation!

dd-mergequeue bot added a commit that referenced this pull request Mar 20, 2024
… into staging-12

Co-authored-by: Benoît Zugmeyer <benoit.zugmeyer@datadoghq.com>
@dd-devflow
Copy link
Contributor

dd-devflow bot commented Mar 20, 2024

🚂 Branch Integration: This commit was successfully integrated

Commit fa634d6162 has been merged into staging-12 in merge commit e451dee258.

Check out the triggered pipeline on Gitlab 🦊

@BenoitZugmeyer
Copy link
Member Author

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Mar 20, 2024

🚂 Branch Integration: starting soon, merge in < 10m

Commit 7398f82784 will soon be integrated into staging-12.

This build is going to start soon! (estimated merge in less than 10m)

Use /to-staging -c to cancel this operation!

dd-mergequeue bot added a commit that referenced this pull request Mar 20, 2024
… into staging-12

Co-authored-by: Benoît Zugmeyer <benoit.zugmeyer@datadoghq.com>
@dd-devflow
Copy link
Contributor

dd-devflow bot commented Mar 20, 2024

🚂 Branch Integration: This commit was successfully integrated

Commit 7398f82784 has been merged into staging-12 in merge commit 456df7459a.

Check out the triggered pipeline on Gitlab 🦊

expect(object.method).toBeUndefined()
})

it('sets an event handler even if it was originally undefined', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

❓ question: ‏ could you explain when this happens? and why we still want to wrap something that was originally undefined?

@BenoitZugmeyer
Copy link
Member Author

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Mar 22, 2024

⚠️ Branch Integration: staging-12 is not configured as an integration branch

We recommend you to add a file named staging.datadog.yml with the following content to register it:

schema-version: v1
kind: integration-branch
name: staging-12
team: <my-team-name>
update_automatically: true
update_at_most_every: 15m
reset_pattern: "0 12 * * Mon"
contacts:
- type: slack
  contact: <my-teams-ops-channel>
---

This configuration would keep your branch up-to-date with the base branch of this repository automatically and would get reset every Monday at noon UTC.

For more information: Integration Branches

If you need support, contact us on Slack #ci-interfaces!

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Mar 22, 2024

🚂 Branch Integration: starting soon, merge in < 10m

Commit 00103f1c9d will soon be integrated into staging-12.

This build is going to start soon! (estimated merge in less than 10m)

Use /to-staging -c to cancel this operation!

dd-mergequeue bot added a commit that referenced this pull request Mar 22, 2024
… into staging-12

Co-authored-by: Benoît Zugmeyer <benoit.zugmeyer@datadoghq.com>
@dd-devflow
Copy link
Contributor

dd-devflow bot commented Mar 22, 2024

🚂 Branch Integration: This commit was successfully integrated

Commit 00103f1c9d has been merged into staging-12 in merge commit 93b5501ad1.

Check out the triggered pipeline on Gitlab 🦊

@BenoitZugmeyer BenoitZugmeyer merged commit 17251cb into main Mar 22, 2024
18 checks passed
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/fix-undefined-instrument-method branch March 22, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants