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

add: test line probe expression language support alongside methods #3877

Merged
merged 8 commits into from
Jan 30, 2025
Merged

add: test line probe expression language support alongside methods #3877

merged 8 commits into from
Jan 30, 2025

Conversation

tylfin
Copy link
Contributor

@tylfin tylfin commented Jan 22, 2025 •
edited
Loading

Motivation

This pull request updates the tests/debugger/test_debugger_expression_language.py file to enhance the setup of expression language tests by incorporating language-specific line numbers and restructuring the probe creation process.

Changes

  • Added language and method variables to various setup methods to facilitate language-specific line number mapping.
  • Introduced _method_and_language_to_line_number method to map methods and languages to their respective line numbers, ensuring accurate probe placement.

Restructuring probe creation:

  • Updated _create_expression_probes method to accept an optional lines parameter and handle both method and line probes. This change facilitates more flexible and comprehensive probe creation.

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

  • If PR title starts with [<language>], double-check that only <language> is impacted by the change
  • No system-tests internal is modified. Otherwise, I have the approval from R&P team
  • CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added (or removed)?

@tylfin tylfin force-pushed the tyler.finethy/expression-lang-line-probes branch 3 times, most recently from 8b2eabe to 45847f8 Compare January 23, 2025 18:10
@tylfin tylfin requested a review from shurivich January 23, 2025 18:39
@tylfin tylfin force-pushed the tyler.finethy/expression-lang-line-probes branch 2 times, most recently from 60e3197 to 9fc3a28 Compare January 27, 2025 16:09
@tylfin tylfin marked this pull request as ready for review January 27, 2025 19:46
@tylfin tylfin requested review from a team as code owners January 27, 2025 19:46
@tylfin tylfin force-pushed the tyler.finethy/expression-lang-line-probes branch from 80e193c to f45baa2 Compare January 27, 2025 19:48
@tylfin tylfin force-pushed the tyler.finethy/expression-lang-line-probes branch from 778d018 to e3ceb7c Compare January 27, 2025 21:31
cbeauchesne
cbeauchesne reviewed Jan 28, 2025
View reviewed changes
Copy link
Collaborator

@cbeauchesne cbeauchesne left a comment

Choose a reason for hiding this comment

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

AGTM form framework usage. Though, I let someone familiar with the feature review the PR.

shurivich
shurivich approved these changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@shurivich shurivich left a comment

Choose a reason for hiding this comment

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

Excellent, love it!

@tylfin
Copy link
Contributor Author

tylfin commented Jan 29, 2025

/merge

@dd-devflow
Copy link

dd-devflow bot commented Jan 29, 2025 •
edited
Loading

Devflow running: /merge

View all feedbacks in Devflow UI.

2025-01-29 14:01:45 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 0s.

2025-01-29 16:02:24 UTC ❌ MergeQueue: The build pipeline has timeout

The merge request has been interrupted because the build took longer than expected. The current limit for the base branch 'main' is 120 minutes.

@DataDog DataDog deleted a comment from dd-devflow bot Jan 29, 2025
@tylfin
Copy link
Contributor Author

tylfin commented Jan 29, 2025

/merge

@dd-devflow
Copy link

dd-devflow bot commented Jan 29, 2025 •
edited
Loading

Devflow running: /merge

View all feedbacks in Devflow UI.

2025-01-29 16:08:15 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

2025-01-29 16:21:19 UTC ℹ️ MergeQueue: merge request added to the queue

The median merge time in main is 0s.

2025-01-29 16:21:33 UTC ❌ MergeQueue: This merge request is not mergeable, blocked by github

PR can't be merged according to github policy

@tylfin
Copy link
Contributor Author

tylfin commented Jan 29, 2025

/merge

@dd-devflow
Copy link

dd-devflow bot commented Jan 29, 2025 •
edited
Loading

Devflow running: /merge

View all feedbacks in Devflow UI.

2025-01-29 17:58:46 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 0s.

2025-01-29 17:59:10 UTC ❌ MergeQueue: This merge request is not mergeable, blocked by github

PR can't be merged according to github policy

@tylfin
Copy link
Contributor Author

tylfin commented Jan 29, 2025

/merge

@dd-devflow
Copy link

dd-devflow bot commented Jan 29, 2025 •
edited
Loading

Devflow running: /merge

View all feedbacks in Devflow UI.

2025-01-29 21:50:21 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 0s.

2025-01-30 13:38:55 UTC ℹ️ MergeQueue: This merge request was already merged

This pull request was merged directly.

OmerRaviv
OmerRaviv approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@OmerRaviv OmerRaviv left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

Just a passing comment - I wonder if at some point it might make sense to change the way these tests work such that we can put inline comments within the sample app code, such as // Probe Here, and have the test code parse these out automatically to find the line numbers - thereby alleviating the need for things like _method_and_language_to_line_number and perhaps making the test a bit easier to reason about.

I suppose that will only really make sense to invest in that if we find ourselves using this pattern a lot more often.

@cbeauchesne
Copy link
Collaborator

@tylfin we're not using mergeQueue, just hit the squash and merge button 😎 !

@tylfin tylfin merged commit debc850 into main Jan 30, 2025
849 of 867 checks passed
@tylfin tylfin deleted the tyler.finethy/expression-lang-line-probes branch January 30, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbeauchesne cbeauchesne cbeauchesne left review comments

@shurivich shurivich shurivich approved these changes

@OmerRaviv OmerRaviv OmerRaviv approved these changes

Assignees
No one assigned
Labels
mergequeue-status: done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tylfin @cbeauchesne @OmerRaviv @shurivich