change(opentelemetry): the default network port for OTLP/HTTP is changed to 4318

[soulbird]

Description

opentelemetry's default network port for OTLP/HTTP is 4318 not 4317.

The default network port for OTLP/gRPC is 4317.

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 first)

[tokers]

@soulbird Shall we also update the corresponding test cases?

[soulbird]

@soulbird Shall we also update the corresponding test cases?
Looks like no corresponding test cases need to be updated.

[tzssangglass]

ref: open-telemetry/opentelemetry-specification#1970

[moonming]

@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？

[moonming]

the title of this PR should add opentelemetry

[soulbird]

@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

[moonming]

@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

[soulbird]

@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.

[moonming]

This port change problem should be covered with test cases

[soulbird]

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

[moonming]

So should this function return an error message?

[spacewander]

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:

apisix/t/plugin/opentelemetry2.t

Line 43 in 0d1280c

client.do_request = function()

.

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 open-telemetry/opentelemetry-specification#1970, it is necessary to change APISIX to adapt to the new break change.

CC the author of lua-opentelemetry and this plugin: @yangxikun

[yangxikun]

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.