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

[OSPP] PHP E2E #11330

Merged
merged 24 commits into from
Oct 10, 2023
Merged

[OSPP] PHP E2E #11330

merged 24 commits into from
Oct 10, 2023

Conversation

simonluo345
Copy link
Contributor

@simonluo345 simonluo345 commented Sep 14, 2023

This pull request introduces changes and improvements to the PHP Docker setup and configuration for SkyWalking agent integration:

  1. Docker Setup:
    Updated to PHP 8.1-fpm-bullseye base image.
    Introduced a builder stage to streamline the build process.
    Installed Rust and its dependencies, a prerequisite for SkyWalking agent build.
    Cloned and set the SkyWalking PHP repository to a specified commit.
    Compiled and installed the SkyWalking agent.
    Used a multi-stage Docker build to improve the final image size and only carry necessary binaries.

  2. Docker Compose:
    Modified image source to be built from the local context.
    Added SW_AGENT_PHP_COMMIT as an argument to pass a specific commit of SkyWalking PHP agent.
    Modified volume mounts for PHP configuration.

  3. Entry Point:
    Introduced an entrypoint script for starting up services (Nginx and PHP-FPM).

  4. Nginx Configuration:
    Added a basic Nginx configuration file to serve PHP applications and pass requests to PHP-FPM.

  5. PHP Configuration:
    Updated the SkyWalking configuration section, segregating settings for general SkyWalking and SkyWalking agent configurations.

Adjusted various settings for better integration with SkyWalking. These changes aim to streamline the PHP setup process, improve the integration with SkyWalking, and provide a more robust development and deployment experience. Feedback and reviews are highly appreciated.

@heyanlong
Copy link
Member

@simonluo345 Could you provide some description?

@wu-sheng
Copy link
Member

@heyanlong From the OSPP perspective, I have concerns about why most commits are made by you rather than a student.
This should not be a recommended and qualified work for a student.

@wu-sheng
Copy link
Member

From upstream/community perspective, I don't care. So, I would not block this PR once it is ready. But I didn't see this kind of commits before.

image

@simonluo345
Copy link
Contributor Author

simonluo345 commented Sep 16, 2023

@heyanlong From the OSPP perspective, I have concerns about why most commits are made by you rather than a student.
This should not be a recommended and qualified work for a student.

I appreciate your input and value your feedback. I understand your concern about the commits.

We are primarily focusing on debugging and resolving any issues that may have arisen during the development process. This involves some code changes to ensure that the project functions correctly. My mentor has been actively involved in this phase to guide and assist me in addressing these issues.

My mentor has been involved in the commits, and this project has provided me with valuable learning experiences. I've been closely working with my mentor to understand the debugging process.

@wu-sheng
Copy link
Member

This PR only includes 100-200 LOCs, it is hard to say it is involved in too many codes.
The 3 months ospp is helping you in trying and learning the project, it should not be a work load for the mentor.

Anyway, this is a reminder. And as a liaison between the org and ospp committee, I have the responsibility to oversight all task execution, and feedback.
Most likely, my concern wouldn't block your project, no matter from project PR or OSPP result. But, I still want to give this concern to you directly and keep transparent.

@simonluo345
Copy link
Contributor Author

This PR only includes 100-200 LOCs, it is hard to say it is involved in too many codes. The 3 months ospp is helping you in trying and learning the project, it should not be a work load for the mentor.

Anyway, this is a reminder. And as a liaison between the org and ospp committee, I have the responsibility to oversight all task execution, and feedback. Most likely, my concern wouldn't block your project, no matter from project PR or OSPP result. But, I still want to give this concern to you directly and keep transparent.

Thank you for pointing out this. I really appreciate it.

@wu-sheng wu-sheng added test Test requirements about performance, feature or before release. agent Language agent related. backend OAP backend related. labels Sep 18, 2023
@wu-sheng
Copy link
Member

Isn't php case added into the GHA control file?

@simonluo345
Copy link
Contributor Author

Isn't php case added into the GHA control file?

Sir could you clarify this? I don't really understand what you mean.

@wu-sheng
Copy link
Member

Never mind. I noticed you updated the PHP case.

heyanlong
heyanlong previously approved these changes Sep 27, 2023
@heyanlong heyanlong requested a review from jmjoy September 27, 2023 09:03
@wu-sheng
Copy link
Member

Remove the WIP prefix if this is ready.

Comment on lines 37 to 40
- key: http.method
value: {{ notEmpty .value }}
- key: http.status_code
value: {{ notEmpty .value }}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe here can be a specific value?

Copy link
Member

Choose a reason for hiding this comment

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

If these are predictable, we should lock the value. This assertion is only about having value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi sir, after I locked the value and ran the local test, the e2e testing was not going to pass because the order of the testing was wrong. I think it is a bug from the e2e testing tool. Can I change it back to {{notEmpty .value}} instead of a fixed value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the screenshot:
WechatIMG1366

Copy link
Member

Choose a reason for hiding this comment

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

@kezhenxu94 Any suggestion here?

Copy link
Member

Choose a reason for hiding this comment

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

@simonluo345 consider pushing your changes so that I can see where is the potential problem, one thing I noticed from the screenshot is that you might wrote some of the expected rules twice like this

image

Not sure though, would be helpful to push the changes here

Copy link
Member

Choose a reason for hiding this comment

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

@simonluo345 Your reply doesn't make sense. We are asking whether your expected data is correct.
Making tests passed is not the purpose. Write the tests correctly and really verify codes, these are the purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wu-sheng Thank you for pointing out this. I misunderstood the question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kezhenxu94 just pushed the changes, could you please take a look?

@simonluo345 simonluo345 changed the title [WIP][OSPP] PHP E2E [OSPP] PHP E2E Sep 27, 2023
@wu-sheng
Copy link
Member

wu-sheng commented Oct 5, 2023

You need to resolve the conflicts, the CI could run after that.

@simonluo345
Copy link
Contributor Author

You need to resolve the conflicts, the CI could run after that.

hi sir, the conflicts are resolved.

@heyanlong heyanlong requested a review from jmjoy October 10, 2023 01:26
@wu-sheng wu-sheng added this to the 9.7.0 milestone Oct 10, 2023
@wu-sheng wu-sheng merged commit 208a9ce into apache:master Oct 10, 2023
162 checks passed
liangyepianzhou pushed a commit to liangyepianzhou/skywalking that referenced this pull request Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent Language agent related. backend OAP backend related. test Test requirements about performance, feature or before release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants