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

feat: adding EXTERNAL_TOOL_DURATION key #488

Merged
merged 1 commit into from
Feb 14, 2025
Merged

feat: adding EXTERNAL_TOOL_DURATION key #488

merged 1 commit into from
Feb 14, 2025

Conversation

miki725
Copy link
Collaborator

@miki725 miki725 commented Feb 12, 2025

Description

To help analyze whats the overhead of the tool, report its duration as a separate key

Testing

➜ maketest test_plugins.py::test_semgrep --pdb

@miki725 miki725 requested a review from viega as a code owner February 12, 2025 18:58
@miki725
Copy link
Collaborator Author

miki725 commented Feb 13, 2025

@mbaltrusitis
Copy link
Collaborator

What do you think of adding a TOTAL_EXTERNAL_TOOL_DURATION key that performs the aggregate?

Copy link
Collaborator

@mbaltrusitis mbaltrusitis left a comment

Choose a reason for hiding this comment

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

LGTM. see comment.

@miki725
Copy link
Collaborator Author

miki725 commented Feb 14, 2025

adding more aggregate for aggregate tool collection sounds like a good idea but we can probably do that in a separate PR. Ill probably merge this for now and we can add additional keys separately.

@miki725 miki725 merged commit e739db0 into main Feb 14, 2025
4 checks passed
@miki725 miki725 deleted the duration branch February 14, 2025 15:55
@mbaltrusitis
Copy link
Collaborator

adding more aggregate for aggregate tool collection sounds like a good idea but we can probably do that in a separate PR. Ill probably merge this for now and we can add additional keys separately.

completely agree

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