You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/04/14 01:27:23 UTC
[GitHub] [skywalking] dmsolr opened a new pull request #6748: [Plugin] add Lua kong component ID
dmsolr opened a new pull request #6748:
URL: https://github.com/apache/skywalking/pull/6748
<!--
⚠️ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
### Add an agent plugin to support <framework name>
- [ ] Add a test case for the new plugin, refer to [the doc](https://github.com/apache/skywalking/blob/master/docs/en/guides/Plugin-test.md)
- [X] Add a component id in [the component-libraries.yml](https://github.com/apache/skywalking/blob/master/oap-server/server-bootstrap/src/main/resources/component-libraries.yml)
- [X] Add a logo in [the UI repo](https://github.com/apache/skywalking-rocketbot-ui/tree/master/src/views/components/topology/assets)
<!-- ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👇 ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking/blob/master/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👆 ==== -->
<!-- ==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👇 ====
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👆 ==== -->
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/CHANGES.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #6748: [Plugin] add Lua kong component ID
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #6748:
URL: https://github.com/apache/skywalking/pull/6748#issuecomment-819173449
- For Java libraries' component IDs, we have this test to verify https://github.com/apache/skywalking/blob/2b9de2007f4ae2004765bbbf0437ee9fe4bcb227/oap-server/server-bootstrap/src/test/java/org/apache/skywalking/oap/server/starter/ComponentLibrariesTest.java#L39 , but in this case, we usually need to modify more file than `component-libraries.yml`, so the CI checks won't be skipped.
- For non-Java components, we have no chance to check them at all actually, for example, malformed YAML like
```yaml
Nginx:
id: 6000
languages: Lua
Kong:
id: 6001
languages: Lua # has one more space here
```
can be detected when parsing the file, but YAML like
```yaml
Nginx:
id: 6000
languages: Lua
Kong: # the entire section is indented as a child of Nginx section
id: 6001
languages: Lua # has one more space here
```
cannot be detected at all because it's also valid YAML and since we only read `Nginx.id` and `Nginx.languages`, and we don't know there is `Kong`, hence the other properties `Kong` is ignored.
Therefore, for non-Java component IDs, malformed YAML cannot be detected unless developers add the E2E tests and put component id assertions in the tests, which I believe we have that currently, for example, Go2Sky E2E, PHP E2E, Python E2E, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #6748: [Plugin] add Lua kong component ID
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #6748:
URL: https://github.com/apache/skywalking/pull/6748#issuecomment-819161092
> Adding component id is common case so I added the file into skip list
The side effect is, if a PR breaks the YAML format, we will face all tests failure on all next PRs :P
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng merged pull request #6748: [Plugin] add Lua kong component ID
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #6748:
URL: https://github.com/apache/skywalking/pull/6748
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #6748: [Plugin] add Lua kong component ID
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #6748:
URL: https://github.com/apache/skywalking/pull/6748#issuecomment-819173910
That said, I prefer non-Java component IDs should be added along with the E2E tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #6748: [Plugin] add Lua kong component ID
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #6748:
URL: https://github.com/apache/skywalking/pull/6748#issuecomment-819170802
> > > Adding component id is common case so I added the file into skip list
> >
> >
> > The side effect is, if a PR breaks the YAML format, we will face all tests failure on all next PRs :P
>
> Yes indeed, this is a tradeoff to reduce the CI resource load while increasing the reviewers' attention, since it's skipped when only `component-libraries.yml` is modified, it's acceptable to review its format as well, let's keep this until we really encounter the case you said.
Do we have a node type check in the topology e2e? If so, we could make sure we will find something wrong in the next PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #6748: [Plugin] add Lua kong component ID
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #6748:
URL: https://github.com/apache/skywalking/pull/6748#issuecomment-819166748
> > > Adding component id is common case so I added the file into skip list
> >
> >
> > The side effect is, if a PR breaks the YAML format, we will face all tests failure on all next PRs :P
>
> Yes indeed, this is a tradeoff to reduce the CI resource load while increasing the reviewers' attention, since it's skipped when only `component-libraries.yml` is modified, it's acceptable to review its format as well, let's keep this until we really encounter the case you said.
Make sense to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #6748: [Plugin] add Lua kong component ID
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #6748:
URL: https://github.com/apache/skywalking/pull/6748#issuecomment-819163335
> > Adding component id is common case so I added the file into skip list
>
> The side effect is, if a PR breaks the YAML format, we will face all tests failure on all next PRs :P
Yes indeed, this is a tradeoff to reduce the CI resource load while increasing the reviewers' attention, since it's skipped when only `component-libraries.yml` is modified, it's acceptable to review its format as well, let's keep this until we really encounter the case you said.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org