Add instrumentation for httpd - Apache HTTP Server #7
Conversation
|
@TomRoSystems - One of the check has failed. Should I merge the PR in case #8 fixes it ? |
I'd give time until end of this week for reviews / comments as this one is bigger. And then merge on Monday. Thanks. |
| static const char *otel_set_exporter(cmd_parms *cmd, void *cfg, const char *arg) | ||
| { | ||
| if (!strcasecmp(arg, "file")) | ||
| config.type = OtelExporterType::OSTREAM; | ||
| else if (!strcasecmp(arg, "otlp")) | ||
| config.type = OtelExporterType::OTLP; | ||
| else | ||
| return "Unknown exporter type - can be file or otlp"; | ||
|
|
||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Please correct me if I'm wrong here, but I think any kind of instrumentation shouldn't define an exporter. Exporters should only be defined by the end-user application developer. Otherwise you could only use one instrumentation at a time (e. g. Apache), but never two or more (e. g. Apache and mysql) as they may conflict (e. g. intitialize two OTEL exporters).
As rule of thumb, instrumentation should only use the OpenTelemetry API, but not the SDK.
There was a problem hiding this comment.
Also, see for example who Go provides instrumentation for its standard HTTP server: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/net/http/otelhttp/example/server/server.go
The exporter is configurable via standard SDK methods by the end-user, and the instrumentation is agnostic of it.
There was a problem hiding this comment.
Just realized that in case of Apache (and nginx) we're not actually instrumenting a library, but and end-user application.
So it might be ok to specify an exporter in this case. However, it's still a bit troubling for me that user can't specify their custom exporters or configure exporters/processors without recompiling.
| @@ -0,0 +1,38 @@ | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
nit: remove heading empty lines?
| namespace | ||
| { | ||
|
|
||
| const std::string kSpanNamePrefix = "HTTP "; |
There was a problem hiding this comment.
Is the trailing space intended?
No description provided.