[receiver/valkey]: Init new component #37005
Conversation
This reverts commit 9898ae9.
rogercoll
force-pushed
the
init_valkey_receiver
branch
from
c3e23c0 to
a64d888
Compare
rogercoll
force-pushed
the
init_valkey_receiver
branch
from
b991b59 to
a82e488
Compare
rogercoll
force-pushed
the
init_valkey_receiver
branch
from
a82e488 to
1644903
Compare
|
cc @dmitryax |
Co-authored-by: Sam DeHaan <[email protected]>
| } | ||
|
|
||
| result, err := vs.client.retrieveInfo(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
Are there errors that should be considered differently - IE errors that show the client is no longer valid and we need to construct a new one?
There was a problem hiding this comment.
Not that I am aware of, the Redis receiver does not do any distinction either https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/redisreceiver/redis_scraper.go#L82
| attrs := make(map[string]string) | ||
| lines := strings.Split(data, attrDelimiter) | ||
| for _, line := range lines { | ||
| if len(line) == 0 || strings.HasPrefix(line, "#") { |
There was a problem hiding this comment.
The lines with a # prefix appear to start a named section of key/value pairs. Is there a promise in the API that there will be no name conflicts across categories? If no, we may need to consider grouping the parsed pairs by category, not in one shared map.
There was a problem hiding this comment.
Since 1.0.0, there do not appear to have been any conflicts in the sections. See the documentation: https://valkey.io/commands/info/
Although I like the idea of grouping the data by section, I would suggest keeping this feature for a future PR if needed. The current exported client metrics are not affected by the tested version (integration test 7.2)." Wdyt?
receiver/valkeyreceiver/README.md
Outdated
|
|
||
| ## Configuration | ||
|
|
||
| > :information_source: This receiver is in beta and configuration fields are subject to change. |
There was a problem hiding this comment.
I think this sentence is redundant, since the status of this component is in development, no one should assume that there will be no breaking changes to configuration.
There was a problem hiding this comment.
Good point, removed in 018a5e4
receiver/valkeyreceiver/README.md
Outdated
| receiver the duration between runs. This value must be a string readable by | ||
| Golang's `ParseDuration` function (example: `1h30m`). Valid time units are | ||
| `ns`, `us` (or `µs`), `ms`, `s`, `m`, `h`. | ||
| - `tls`: |
There was a problem hiding this comment.
I think you can just link the Readme for configtls https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configtls/README.md
There was a problem hiding this comment.
Makes sense, thanks 018a5e4
.chloggen/init_valkey_receiver.yaml
Outdated
| # Use this changelog template to create an entry for release notes. | ||
|
|
||
| # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
| change_type: 'enhancement' |
There was a problem hiding this comment.
this should be new_component
There was a problem hiding this comment.
Could you fix the conflict?
And there is also a failed lint
https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/12650162873/job/35248291762?pr=37005
Running target 'lint' in module 'receiver/valkeyreceiver' as part of group 'receiver-3'
make --no-print-directory -C receiver/valkeyreceiver lint
go: downloading go.opentelemetry.io/otel/metric v1.33.0
go: downloading go.opentelemetry.io/otel/trace v1.33.0
go: downloading github.com/valkey-io/valkey-go v1.0.45
go: downloading go.opentelemetry.io/otel v1.33.0
go: downloading google.golang.org/genproto/googleapis/rpc v0.0.0-202411182[336](https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/12650162873/job/35248291762?pr=37005#step:8:337)22-e639e219e697
Check License finished successfully
running /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/misspell -error
/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/golangci-lint run --allow-parallel-runners --build-tags integration --timeout=30m --path-prefix valkeyreceiver
level=error msg="Running error: context loading failed: no go files to analyze: running `go mod tidy` may solve the problem"
29e109c to
8309e1c
Compare
| next consumer. The `collection_interval` configuration option tells this | ||
| receiver the duration between runs. This value must be a string readable by | ||
| Golang's `ParseDuration` function (example: `1h30m`). Valid time units are | ||
| `ns`, `us` (or `µs`), `ms`, `s`, `m`, `h`. |
There was a problem hiding this comment.
Small nit: I think maybe it's not reasonable to set up a too shot collection interval to prevent too much stress on Valkey.
Therefore, we can set up to minimum duration check in code to validate it.
There was a problem hiding this comment.
Looks reasonable to me, which low bound would you propose? Maybe 1 second?
Do we have any other scraper that sets a minimum collection interval value?
There was a problem hiding this comment.
Sorry I'm not sure about this, but 1s looks good to me, even though receiver can fetch metric the at such granularity, but I think in most cases, the user won't store or query the metrics in that granularity. For me I often query the metric in the granularity of one minute. It's enough for me in most cases.
Description
Base receiver code for the new Valkey receiver (metadata, client interface, unit and integration tests). Just the
valkey.client.connection.countmetric will be available after this PR. Based on https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-metrics.md#metric-dbclientconnectioncountConfiguration options: Valkey endpoint + TLS options
Link to tracking issue
Fixes #33787
Testing
testdata+ contrib golden package.Documentation