[exporter/azuremonitorexporter] Regression issue, property endpoint is ignored when using instrumentation_key

[michaelkira]

Component(s)

exporter/azuremonitor

What happened?

Description

When we upgrade collector version from 0.56.0 to 0.96.0, we find we no longer export trace info when using
endpoint:
https://dc.applicationinsights.azure.cn/v2/track

Checking the code, it seems when connectionString is null, the collector will take the DefaultIngestionEndpoint and ignored passed in endpoint.
This cause we not able to drop data to mooncake application insights

opentelemetry-collector-contrib/exporter/azuremonitorexporter/connection_string_parser.go

Line 47 in 6575f8a

connectionVars.IngestionURL = getIngestionURL(DefaultIngestionEndpoint)

Steps to Reproduce

Use 0.96.0
Pass in
endpoint: https://dc.applicationinsights.azure.cn/v2/track
instrumentation_key: your key

Expected Result

The trace data ingested into mooncake app insights

Actual Result

No data ingested

Collector version

0.96.0

Environment information

Environment

OS: (e.g., "Ubuntu 20.04")
Compiler(if manually compiled): (e.g., "go 14.2")

OpenTelemetry Collector configuration

apiVersion: v1
data:
  otel-collector-config: |
    receivers:
      zipkin:
        endpoint: 0.0.0.0:9411
    extensions:
      health_check:
      pprof:
        endpoint: :1888
      zpages:
        endpoint: :55679
    exporters:
      logging:
        loglevel: debug
      azuremonitor:
        endpoint:
https://dc.applicationinsights.azure.cn/v2/track
        instrumentation_key: your key
        maxbatchsize: 100
        maxbatchinterval: 5s
    service:
      extensions: [pprof, zpages, health_check]
      pipelines:
        traces:
          receivers: [zipkin]
          exporters: [azuremonitor,logging]
kind: ConfigMap

Log output

No error log

Additional context

No response

[github-actions]

Pinging code owners:

• exporter/azuremonitor: @pcwiese

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[michaelkira]

Any feedback?

[hgaol]

Hi @michaelkira , looks it's quite straightforward and I've created PR to fix it.

[hgaol]

BTW, I also found it has been hard coded to replace the path of url with /v2.1/track. indicated below,

opentelemetry-collector-contrib/exporter/azuremonitorexporter/connection_string_parser.go

Line 87 in 2027679

ingestionURL.Path = path.Join(ingestionURL.Path, "/v2.1/track")

Is it what you expected? @michaelkira

Hi @pcwiese , is there any reason for this? If not, I thought it's more flexible to let user specify the value. What do you think of it?

[michaelkira]

BTW, I also found it has been hard coded to replace the path of url with /v2.1/track. indicated below,

opentelemetry-collector-contrib/exporter/azuremonitorexporter/connection_string_parser.go

Line 87 in 2027679

ingestionURL.Path = path.Join(ingestionURL.Path, "/v2.1/track")

Is it what you expected? @michaelkira

Hi @pcwiese , is there any reason for this? If not, I thought it's more flexible to let user specify the value. What do you think of it?
Hi @hgaol I am not familiar with the version difference here. The current version works good for global endpoint, so I assume its expected.
Btw, is the fixed merged? Which version should I ref to?

[hgaol]

BTW, I also found it has been hard coded to replace the path of url with /v2.1/track. indicated below,

opentelemetry-collector-contrib/exporter/azuremonitorexporter/connection_string_parser.go

Line 87 in 2027679

ingestionURL.Path = path.Join(ingestionURL.Path, "/v2.1/track")

Is it what you expected? @michaelkira
Hi @pcwiese , is there any reason for this? If not, I thought it's more flexible to let user specify the value. What do you think of it?
Hi @hgaol I am not familiar with the version difference here. The current version works good for global endpoint, so I assume its expected.
Btw, is the fixed merged? Which version should I ref to?

Hi @michaelkira , I've created a PR to fix it. But it needs the component owner @pcwiese to help to review before merge.

[michaelkira]

BTW, I also found it has been hard coded to replace the path of url with /v2.1/track. indicated below,

opentelemetry-collector-contrib/exporter/azuremonitorexporter/connection_string_parser.go

Line 87 in 2027679

ingestionURL.Path = path.Join(ingestionURL.Path, "/v2.1/track")

Is it what you expected? @michaelkira
Hi @pcwiese , is there any reason for this? If not, I thought it's more flexible to let user specify the value. What do you think of it?
Hi @hgaol I am not familiar with the version difference here. The current version works good for global endpoint, so I assume its expected.
Btw, is the fixed merged? Which version should I ref to?

Hi @michaelkira , I've created a PR to fix it. But it needs the component owner @pcwiese to help to review before merge.

Hi @hgaol Thanks for the fix, I see the pr already approved, can you merge the pr can let us know how can we get test image to verify?

[hgaol]

Hi @michaelkira , I have no permission to merge and I guess only maintainer and approver can do it. Since both owner and maintainer has approved it and all tests have passed, I think they'll merge it once at convenience. Or you can ping them in the PR if it's urgent for you.

[atoulme]

It looks like this issue was resolved, please close the issue if this is confirmed.

[github-actions]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

• exporter/azuremonitor: @pcwiese

See Adding Labels via Comments if you do not have permissions to add labels yourself.