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

[APPSEC-9935] AppSec, make sure to use service entry span. #2898

Merged

Conversation

GustavoCaso
Copy link
Member

@GustavoCaso GustavoCaso commented Jun 7, 2023

What does this PR do?

Use the scope.service_entry_span when tagging a span with appsec related tags.

Motivation

Make sure AppSec features work as expected, and confront to the system tests specs

Additional Notes

How to test the change?

CI

@github-actions github-actions bot added appsec Application Security monitoring product integrations Involves tracing integrations labels Jun 7, 2023
@GustavoCaso GustavoCaso force-pushed the appsec-make-sure-tags-are-added-to-service-entry-span branch from 2245f92 to a8cbb46 Compare June 7, 2023 08:07
@GustavoCaso GustavoCaso changed the title AppSec. Make sure to add span tags to service entry span. [APPSEC-9935] AppSec, make sure to add span tags to service entry span. Jun 7, 2023
@GustavoCaso GustavoCaso marked this pull request as ready for review June 7, 2023 11:24
@GustavoCaso GustavoCaso requested review from a team and lloeki June 7, 2023 11:24
@GustavoCaso GustavoCaso changed the title [APPSEC-9935] AppSec, make sure to add span tags to service entry span. [APPSEC-9935] AppSec, make sure to use service entry span. Jun 7, 2023
Copy link
Contributor

@lloeki lloeki left a comment

Choose a reason for hiding this comment

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

LGTM code-wise.

That said, I did not do it on the original PR that tagged on service entry span because I thought it was specified that these tags should be on the closest span where the event happened, not the service entry span.

Therefore if it is so, and since this appears to be borne out of system tests failing, I'm questioning whether the system tests actually respect the RFC.

@GustavoCaso GustavoCaso merged commit 132c814 into master Jun 9, 2023
202 checks passed
@GustavoCaso GustavoCaso deleted the appsec-make-sure-tags-are-added-to-service-entry-span branch June 9, 2023 07:28
@github-actions github-actions bot added this to the 1.13.0 milestone Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appsec Application Security monitoring product integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants