feat: add option to send forwarded metrics to Observe via OTLP #212
Conversation
![orca-security-us[bot]](https://avatars.githubusercontent.com/in/276250?s=60&v=4){kind=small}
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
 Passed |
Infrastructure as Code |  0  0  0  0 |
View in Orca |
| Passed |
SAST | 0 0 0 0 |
View in Orca |
| Passed |
Secrets | 0 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
obs-gh-mattcotter
force-pushed
the
mc/otel-metrics-forward
branch
2 times, most recently
from
b52a37e to
503a0d4
Compare
| authorization: ${env:OBSERVE_AUTHORIZATION_HEADER} | ||
| x-observe-target-package: "Metrics" | ||
| sending_queue: | ||
| num_consumers: 4 |
There was a problem hiding this comment.
how'd you land on these values?
There was a problem hiding this comment.
These are the same values used in all of our otlphttp exporters. It's lower than the default, but I don't know how we arrived at them in the first place. Do you think we should make them configurable in the agent config?
There was a problem hiding this comment.
dont need to in this PR
| # Enable forwarding of local app metrics and traces | ||
| forwarding: | ||
| enabled: true | ||
| otlp_metrics: true |
There was a problem hiding this comment.
its a little unclear that these are referring to the output format; maybe we should nest this in another object something like
forwarding:
enabled: true
output:
metrics_format: "prometheus" or "OTLP"
There was a problem hiding this comment.
I went with:
forwarding:
enabled: true
metrics:
output_format: "otlp"to match your helm change (but kept snake case like the rest of the agent config).
There was a problem hiding this comment.
should the format be otel or otlp? the latter is more specific but references the protocol vs. the actual schema/data format
There was a problem hiding this comment.
I think otel make sense; I liked that in the helm chart. Updated!
| configFile := v.ConfigFileUsed() | ||
| if configFile == "" { | ||
| return false, nil, fmt.Errorf("no config file defined") | ||
| } | ||
| if _, err := os.Stat(configFile); err != nil && errors.Is(err, os.ErrNotExist) { |
There was a problem hiding this comment.
this seems like an unrelated change? should it be in the same PR?
There was a problem hiding this comment.
The diagnose check wasn't previously applying our config defaulting logic. Since this change adds a config option with a default that's invalid when empty, diagnose needed to be updated. So it's a loosely related bugfix that needs to happen before this option is added. I can split it out if you prefer?
obs-gh-mattcotter
force-pushed
the
mc/otel-metrics-forward
branch
from
c637fcb to
9b5d0b4
Compare
Description
Add option to send forwarded metrics to Observe via OTLP. Going forward, we want to be able to send all custom metrics to Observe in the format the user prefers (between OTLP or Prometheus)