-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added CollectD JSON receiver #44
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase.
"github.com/golang/protobuf/ptypes/timestamp" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded blank line.
receiver/collectdreceiver/factory.go
Outdated
"time" | ||
|
||
"go.uber.org/zap" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blank line.
e19315f
to
a01f119
Compare
fb26eb4
to
6e004fa
Compare
This is really a receiver for the collectd write_http plugin with the Also this is a super-set of the protocol that supports arbitrary dimensions via the square bracket, comma separated key value pairs appended to various fields of the value, which should also be clarified in the doc for this receiver as it is technically not fully backwards compatible with the original protocol since you could legally have square brackets in the value fields that could result in unintended conversions for people that weren't using the SignalFx-specific form of collectd. |
1e954d0
to
dc833e8
Compare
Updated. @pjanotti please take a look as well when you can. |
dc833e8
to
ed54918
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general, a bunch of Qs and nits.
lKeys, lValues := labelKeysAndValues(labels) | ||
metric.MetricDescriptor = &metricspb.MetricDescriptor{ | ||
Name: name, | ||
Type: r.metricType(dsType, isDouble), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: this bothers me on the proto: the point has the value but the metric descriptor too... so the format itself allow inconsistencies... am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. In the sense that descriptor can say the metric is let's say commulative double but each point can be then be either int or double.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tigrannajaryan was the above improved in OTel metrics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not improved. Otel metrics still has a type enum in the decsriptor and then a set of fields to store data points. It is possible to misuse it the same way we have here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth opening an issue on spec repo? Or there are reasons for that?
978a2b3
to
c6389c4
Compare
Thanks for the detailed review @pjanotti. Updated the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please check the suggestion regarding the README.md
lKeys, lValues := labelKeysAndValues(labels) | ||
metric.MetricDescriptor = &metricspb.MetricDescriptor{ | ||
Name: name, | ||
Type: r.metricType(dsType, isDouble), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tigrannajaryan was the above improved in OTel metrics?
c6389c4
to
d9ab2f2
Compare
Ported SignalFx CollectD receiver from SignalFX Gateway to OpenTelemetry Collector Ported from: https://github.com/signalfx/gateway/tree/master/protocol/collectd
d9ab2f2
to
08bdbd0
Compare
[Closes #43]
The files removed in this change already have instrumentation in the opentelemetry-python repo. Removing them from this reference folder as they're no longer needed.
…pen-telemetry#44) * Modify resourcedetection processor to use our aws-cwa-dev confighttp
Ported CollectD receiver from SignalFX Gateway to OpenTelemetry Collector
Ported from: https://github.com/signalfx/gateway/tree/master/protocol/collectd