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

Add E2E coverage for Trace and Logging #199

Merged
merged 15 commits into from Mar 30, 2022
Merged

Conversation

Superskyyy
Copy link
Member

@Superskyyy Superskyyy commented Mar 28, 2022

  • Change Fastapi to FastAPI component name (sync with OAP change)
  • Add E2E coverage for Python agent (Trace and Logging), profiling is not tested for now.
  • Renamed a build variable to remove possible mix-ups with CLI environment Var.
  • Enhance plugin test to reuse images.
  • Combined CI to pass if changes are non-essential to agent and workflows.

Note: There's a flaky test due to a possible {{- contains }} issue in the Infra-e2e verifier, the logs seem to be queried in an unstable order by the CLI where later log often placed first in log list, combining with the contains bug, it becomes flaky.

A time.sleep() workaround makes sure the log arrives in clear order which passes the e2e for now.

@Superskyyy Superskyyy added the test test label Mar 28, 2022
Comment on lines 25 to 32
paths-ignore:
- "*.md"
- "*.txt"
- ".asf.yaml"
- ".dlc.json"
- ".licenserc.yaml"
- "docs/*"
- ".github/workflows/*"
Copy link
Member

Choose a reason for hiding this comment

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

Pull requests require the CI check status context named CheckStatus to pass before they can be merged. And the check context is generated by the job CheckStatus in this file (line 123), if changes are in these paths-ignore, the workflow won't be run and the check status context won't be reported, thus all PRs can't be merged, you need to either remove these paths-ignore, or add a dummy workflow in a separate file that run a empty job named CheckStatus to generate the required context, but it is hard to set the reversed condition of these paths-ignore, you can see how I set in the main repo but that is still not perfect because under some cases the 2 workflows are all executed

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see how it works, gonna fix it later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pull requests require the CI check status context named CheckStatus to pass before they can be merged. And the check context is generated by the job CheckStatus in this file (line 123), if changes are in these paths-ignore, the workflow won't be run and the check status context won't be reported, thus all PRs can't be merged, you need to either remove these paths-ignore, or add a dummy workflow in a separate file that run a empty job named CheckStatus to generate the required context, but it is hard to set the reversed condition of these paths-ignore, you can see how I set in the main repo but that is still not perfect because under some cases the 2 workflows are all executed

Looks like the paths_ignore option doesn't work like what I thought.. it's crazy.

Maybe the diff method is the ultimate solution given third party action cannot be run in asf repos.

Diff method:
https://github.community/t/paths-ignore-not-working-for-base-level-files/18445/4

Copy link
Member

Choose a reason for hiding this comment

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

Some third-party GHAs are approved by Apache infra and can be used, @Superskyyy take a look at this, though I haven't tried to use this GHA to achieve my goal as stated above

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm gonna try it and let you know if it works.

docker/Dockerfile Outdated Show resolved Hide resolved
Signed-off-by: Superskyyy <superskyyy@outlook.com>
@Superskyyy Superskyyy marked this pull request as draft March 28, 2022 16:01
Signed-off-by: Superskyyy <superskyyy@outlook.com>
Signed-off-by: Superskyyy <superskyyy@outlook.com>
Signed-off-by: Superskyyy <superskyyy@outlook.com>
Signed-off-by: Superskyyy <superskyyy@outlook.com>
Signed-off-by: Superskyyy <superskyyy@outlook.com>
Signed-off-by: Superskyyy <superskyyy@outlook.com>
Signed-off-by: Superskyyy <superskyyy@outlook.com>
Signed-off-by: Superskyyy <superskyyy@outlook.com>
Signed-off-by: Superskyyy <superskyyy@outlook.com>
@Superskyyy
Copy link
Member Author

Superskyyy commented Mar 30, 2022

Changes to non-essential files passes the statuscheck with skipping
image
Any failure fails the statuscheck
image
Modification to essential files triggers whole process
image

I think it is complete now. @kezhenxu94

@Superskyyy Superskyyy marked this pull request as ready for review March 30, 2022 00:15
Signed-off-by: Superskyyy <superskyyy@outlook.com>
Signed-off-by: Superskyyy <superskyyy@outlook.com>
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

I left some comments but generally is good to me

tests/e2e/base/docker-compose.base.yml Outdated Show resolved Hide resolved
tests/e2e/case/grpc/e2e.yaml Outdated Show resolved Hide resolved
tests/e2e/case/kafka/e2e.yaml Outdated Show resolved Hide resolved
tests/e2e/script/env Show resolved Hide resolved
.github/workflows/CI.yaml Show resolved Hide resolved
Signed-off-by: Superskyyy <superskyyy@outlook.com>
@Superskyyy
Copy link
Member Author

All fixed according to comments :)

@kezhenxu94 kezhenxu94 added this to the 1.0.0 milestone Mar 30, 2022
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

Thanks!

@kezhenxu94 kezhenxu94 merged commit 95f4101 into apache:master Mar 30, 2022
@Superskyyy Superskyyy deleted the e2e branch March 30, 2022 02:21
@wu-sheng
Copy link
Member

@Superskyyy I think an issue is relative to this? Should we close that?

@Superskyyy
Copy link
Member Author

Superskyyy commented Mar 30, 2022

@Superskyyy I think an issue is relative to this? Should we close that?

There's the profiling feature not yet tested by E2E, but I noticed the Java agent also didn't test it, was it not necessary?

@wu-sheng
Copy link
Member

@Superskyyy
Copy link
Member Author

https://github.com/apache/skywalking/blob/master/.github/workflows/skywalking.yaml#L388 agent profiling is tested in main repo.

Ok I see it now, I got confused last time I checked the profiling e2e parts.. 😄
Let me close that issue first, I'm gonna prioritize the SWCK agent initcontainer task then come back to this.

@Superskyyy
Copy link
Member Author

Create ref link: apache/skywalking#7708

@kezhenxu94
Copy link
Member

@Superskyyy Ci failed in master branch https://github.com/apache/skywalking-python/runs/5747416666?check_suite_focus=true , wanna take a look?

@Superskyyy
Copy link
Member Author

@Superskyyy Ci failed in master branch apache/skywalking-python/runs/5747416666?check_suite_focus=true , wanna take a look?

Yes! Fixing along with another small enhancement in log reporter.. testing on my branch now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test test
Projects
None yet
3 participants