You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/05/08 14:17:46 UTC
[GitHub] [apisix] soulbird opened a new pull request, #7007: chore: the default network port for OTLP/HTTP is 4318
soulbird opened a new pull request, #7007:
URL: https://github.com/apache/apisix/pull/7007
### Description
opentelemetry's default network port for OTLP/HTTP is 4318 not 4317.
The default network port for OTLP/gRPC is 4317.
Fixes # (issue)
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on pull request #7007: chore(opentelemetry): the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
soulbird commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120551721
> > > > > @soulbird Shall we also update the corresponding test cases?
> > > > > Looks like no corresponding test cases need to be updated.
> > >
> > >
> > > Why？ No test cases for opentelemetry？
> >
> >
> > In the test cases of opentelemetry, the data is reported to the black hole, and there is no port-related test. Look: https://github.com/apache/apisix/blob/master/t/plugin/opentelemetry.t#L41
>
> Is opentelemetry really started? If yes, there should be has port
No services related to opentelemetry were found to be started.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #7007: chore: the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
moonming commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120545816
>
Why？ No test cases for opentelemetry？
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #7007: change(opentelemetry): the default network port for OTLP/HTTP is changed to 4318
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120656844
> > This port change problem should be covered with test cases
>
> I think it is not necessary to add test coverage related to the default port, and the success or failure of the report will not affect the operation of APISIX. You can see that the `do_request` function does not return any relevant error information, it just output a log. Look: https://github.com/yangxikun/opentelemetry-lua/blob/main/lib/opentelemetry/trace/exporter/http_client.lua#L30 cc @spacewander
In the test, the `do_request` is replaced to a stub: https://github.com/apache/apisix/blob/0d1280cde7fcd463f2e86941935e053561bcf331/t/plugin/opentelemetry2.t#L43.
So changing the port won't affect the behavior of the test, and the correctness of trace reporting is covered by lua-opentelemetry itself.
On the other hand, as the default network port for OTLP/HTTP is changed from 4317 to 4318 in https://github.com/open-telemetry/opentelemetry-specification/pull/1970, it is necessary to change APISIX to adapt to the new break change.
CC the author of lua-opentelemetry and this plugin: @yangxikun
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #7007: chore(opentelemetry): the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
moonming commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120550537
> > > > @soulbird Shall we also update the corresponding test cases?
> > > > Looks like no corresponding test cases need to be updated.
> >
> >
> > Why？ No test cases for opentelemetry？
>
> In the test cases of opentelemetry, the data is reported to the black hole, and there is no port-related test. Look: https://github.com/apache/apisix/blob/master/t/plugin/opentelemetry.t#L41
Is opentelemetry really started? If yes, there should be has port
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #7007: chore: the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
moonming commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120546546
the title of this PR should add `opentelemetry`
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #7007: change(opentelemetry): the default network port for OTLP/HTTP is changed to 4318
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7007:
URL: https://github.com/apache/apisix/pull/7007
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on pull request #7007: chore: the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
soulbird commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120548785
> opentelemetry
In the test cases of opentelemetry, the data is reported to the black hole, and there is no port-related test. Look: https://github.com/apache/apisix/blob/master/t/plugin/opentelemetry.t#L41
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #7007: chore: the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120544385
ref: https://github.com/open-telemetry/opentelemetry-specification/pull/1970
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #7007: chore: the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
moonming commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120545989
> > @soulbird Shall we also update the corresponding test cases?
> > Looks like no corresponding test cases need to be updated.
Why？ No test cases for opentelemetry？
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #7007: chore(opentelemetry): the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
moonming commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120557376
This port change problem should be covered with test cases
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on pull request #7007: chore(opentelemetry): the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
soulbird commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120595219
> This port change problem should be covered with test cases
I think it is not necessary to add test coverage related to the default port, and the success or failure of the report will not affect the operation of APISIX. You can see that the `do_request` function does not return any relevant error information, it just output a log. Look: https://github.com/yangxikun/opentelemetry-lua/blob/main/lib/opentelemetry/trace/exporter/http_client.lua#L30
cc @spacewander
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on pull request #7007: chore(opentelemetry): the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
soulbird commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120595331
> This port change problem should be covered with test cases
I think it is not necessary to add test coverage related to the default port, and the success or failure of the report will not affect the operation of APISIX. You can see that the `do_request` function does not return any relevant error information, it just output a log. Look: https://github.com/yangxikun/opentelemetry-lua/blob/main/lib/opentelemetry/trace/exporter/http_client.lua#L30
cc @spacewander
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #7007: chore: the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
tokers commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120529886
@soulbird Shall we also update the corresponding test cases?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] yangxikun commented on pull request #7007: change(opentelemetry): the default network port for OTLP/HTTP is changed to 4318
Posted by GitBox <gi...@apache.org>.
yangxikun commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1122104638
Maybe its my careless to use 4317 port as default value. Change 4317 to 4318 is fine. Users usually configure the plugin option `collector.address` explicitly instead of using the default values.
I think test case is not need for this case.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on pull request #7007: chore: the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
soulbird commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120532356
> @soulbird Shall we also update the corresponding test cases?
Looks like no corresponding test cases need to be updated.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on pull request #7007: chore: the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
soulbird commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120549098
> > > @soulbird Shall we also update the corresponding test cases?
> > > Looks like no corresponding test cases need to be updated.
>
> Why？ No test cases for opentelemetry？
In the test cases of opentelemetry, the data is reported to the black hole, and there is no port-related test. Look: https://github.com/apache/apisix/blob/master/t/plugin/opentelemetry.t#L41
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #7007: chore(opentelemetry): the default network port for OTLP/HTTP is 4318
Posted by GitBox <gi...@apache.org>.
moonming commented on PR #7007:
URL: https://github.com/apache/apisix/pull/7007#issuecomment-1120603038
>
So should this function return an error message?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org