Add Collector version to Prometheus Remote Write Exporter user-agent header #3094
Conversation
…-o11y/opentelemetry-collector into update-prwexporter-useragent
|
Requesting reviews from @Aneurysm9 @anuraaga |
dhruv-vora
changed the title
| ) | ||
|
|
||
| const ( | ||
| maxConcurrentRequests = 5 | ||
| maxBatchByteSize = 3000000 | ||
| prwVersion = "0.1.0" |
There was a problem hiding this comment.
Why defining this version? Why not using the version from the BuildInfo?
There was a problem hiding this comment.
The version from the BuildInfo would be the version of the Collector. The prwVersion is for the X-Prometheus-Remote-Write-Version. Hence, I have defined this version separately.
There was a problem hiding this comment.
It appears to be the format of the data, something like a content encoding.
But do you need it in the user agent? It's already sent as a header.
There was a problem hiding this comment.
We need this information as we are using only the User-Agent header information in the back-end AMP service
| @@ -321,8 +327,8 @@ func (prwe *PrwExporter) execute(ctx context.Context, writeReq *prompb.WriteRequ | |||
| // https://cortexmetrics.io/docs/apis/#remote-api | |||
| req.Header.Add("Content-Encoding", "snappy") | |||
| req.Header.Set("Content-Type", "application/x-protobuf") | |||
| req.Header.Set("X-Prometheus-Remote-Write-Version", "0.1.0") | |||
| req.Header.Set("User-Agent", "OpenTelemetry-Collector/"+version.Version) | |||
| req.Header.Set("X-Prometheus-Remote-Write-Version", prwVersion) | |||
There was a problem hiding this comment.
We need this information as we are using only the User-Agent header information in the back-end AMP service
If this is true, can we remove this line?
There was a problem hiding this comment.
I do not think that we can remove that line as per the requirement in https://cortexmetrics.io/docs/api/#remote-write.
There was a problem hiding this comment.
So if it's sent to the backend here we don't need it in the user agent right? It's a bit weird to have the same information in two headers, it's better if the backend can do what it needs based on all the headers.
There was a problem hiding this comment.
Yes, I understand that. Actually the AMP backend sends the logs to CloudWatch logs and in these logs, only the user-agent field is present. That was the reason I added the X-Prometheus-Remote-Write-Version information in the user-agent header. @alolita and @Aneurysm9, could you take a look at this to determine if X-Prometheus-Remote-Write-Version information is required in the user-agent header. If not, I can remove it
There was a problem hiding this comment.
X-Prometheus-Remote-Write-Version needs to be a distinct header per the PRW spec. I'm not sure it needs to be in the user agent header, though. Particularly if prwVersion is a constant, as it is here, we can derive the PRW version from the collector version. If that ever changes we can re-evaluate in the future.
There was a problem hiding this comment.
We want it in the user agent header as the other distinct header is not present in the CW Logs from AMP, hence adding it in the user agent header would be way to get this information. Also about deriving the prwVersion from the collector version, I do not think they are related and so I don't think it can be derived
There was a problem hiding this comment.
- the AMP log processing pipeline is an AWS internal detail that we can change independent of this exporter, the relevant information is in the HTTP request.
- the collector implements a single version of the PRW protocol and there is no indication that that will change, even if the PRW protocol version changes. There will be a 1:1 correspondence between collector version and supported PRW protocol version. Therefore, the information that would be conveyed by having the PRW version data in the user agent header is already present in the collector version data.
There was a problem hiding this comment.
I agree there will be a 1:1 correspondence between the Collector version and supported PRW protocol version information.
…header (open-telemetry#3094) * Add collector version to prometheus remote write exporter user agent * Add collector version to prometheus remote write exporter user agent * refined user-agent header and tests * pre-computed user-agent and updated functions * removed startInfo from PrwExporter struct * removed usage of GitHash * Add collector version to prometheus remote write exporter user agent * Add collector version to prometheus remote write exporter user agent * refined user-agent header and tests * pre-computed user-agent and updated functions * removed startInfo from PrwExporter struct * removed usage of GitHash * updated to use BuildInfo and LongName * renamed variables to use new convention * removed X-Prometheus-Remote-Write-Version/0.1.0 from User-Agent header Co-authored-by: Anthony J Mirabella <a9@aneurysm9.com>
…header (open-telemetry#3094) * Add collector version to prometheus remote write exporter user agent * Add collector version to prometheus remote write exporter user agent * refined user-agent header and tests * pre-computed user-agent and updated functions * removed startInfo from PrwExporter struct * removed usage of GitHash * Add collector version to prometheus remote write exporter user agent * Add collector version to prometheus remote write exporter user agent * refined user-agent header and tests * pre-computed user-agent and updated functions * removed startInfo from PrwExporter struct * removed usage of GitHash * updated to use BuildInfo and LongName * renamed variables to use new convention * removed X-Prometheus-Remote-Write-Version/0.1.0 from User-Agent header Co-authored-by: Anthony J Mirabella <a9@aneurysm9.com>
…header (open-telemetry#3094) * Add collector version to prometheus remote write exporter user agent * Add collector version to prometheus remote write exporter user agent * refined user-agent header and tests * pre-computed user-agent and updated functions * removed startInfo from PrwExporter struct * removed usage of GitHash * Add collector version to prometheus remote write exporter user agent * Add collector version to prometheus remote write exporter user agent * refined user-agent header and tests * pre-computed user-agent and updated functions * removed startInfo from PrwExporter struct * removed usage of GitHash * updated to use BuildInfo and LongName * renamed variables to use new convention * removed X-Prometheus-Remote-Write-Version/0.1.0 from User-Agent header Co-authored-by: Anthony J Mirabella <a9@aneurysm9.com>
Description:
In order to better understand usage patterns for the Prometheus Remote Write exporter in the Collector, we’re adding the Collector name and version info to the user-agent request header in the Prometheus Remote Write exporter.