Skip to content
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

[filestorage] receiver name sanitization #20731

Closed
newly12 opened this issue Apr 6, 2023 · 11 comments · Fixed by #20896
Closed

[filestorage] receiver name sanitization #20731

newly12 opened this issue Apr 6, 2023 · 11 comments · Fixed by #20896
Assignees

Comments

@newly12
Copy link
Contributor

newly12 commented Apr 6, 2023

Component(s)

No response

Is your feature request related to a problem? Please describe.

/ might be used in receiver name, for example, filelog/pod/container/stdout, when storage is enabled(for persisting file fingerprints, offsets, etc), the path will be receiver_filelog_pod/container/stdout, and dir receiver_filelog_pod/container will need to be created first, I think it makes sense to avoid such issue via the sanitization instead of changing receiver name.

if name == "" {
rawName = fmt.Sprintf("%s_%s_%s", kindString(kind), ent.Type(), ent.Name())
} else {
rawName = fmt.Sprintf("%s_%s_%s_%s", kindString(kind), ent.Type(), ent.Name(), name)
}
// TODO sanitize rawName
absoluteName := filepath.Join(lfs.cfg.Directory, rawName)

Describe the solution you'd like

replace / with ~ in receiver name, looks like such convention is used in some k8s components, for example

  • pods/volumes/kubernetes.io~csi
  • pods/volumes/kubernetes.io~empty~dir
  • pods/volumes/kubernetes.io~secrets

Describe alternatives you've considered

No response

Additional context

No response

@newly12 newly12 added enhancement New feature or request needs triage New item requiring triage labels Apr 6, 2023
@andrzej-stencel
Copy link
Member

I can confirm this is an issue, here's a sample config:

exporters:
  logging:

extensions:
  file_storage:
    directory: .

receivers:
  filelog/logs/json:
    include: ./logs-json.log
    start_at: beginning
    storage: file_storage

service:
  extensions:
  - file_storage
  pipelines:
    logs:
      exporters:
      - logging
      receivers:
      - filelog/logs/json

and the output for the current latest v0.75.0

$ ./otelcol-contrib-0.75.0-darwin_amd64 --config ./config.yaml
2023-04-06T17:03:18.713+0200    info    service/telemetry.go:90 Setting up own telemetry...
2023-04-06T17:03:18.713+0200    info    service/telemetry.go:116        Serving Prometheus metrics      {"address": ":8888", "level": "Basic"}
2023-04-06T17:03:18.713+0200    info    [email protected]/exporter.go:286        Development component. May change in the future.        {"kind": "exporter", "data_type": "logs", "name": "logging"}
2023-04-06T17:03:18.715+0200    info    service/service.go:129  Starting otelcol-contrib...     {"Version": "0.75.0", "NumCPU": 16}
2023-04-06T17:03:18.715+0200    info    extensions/extensions.go:41     Starting extensions...
2023-04-06T17:03:18.715+0200    info    extensions/extensions.go:44     Extension is starting...        {"kind": "extension", "name": "file_storage"}
2023-04-06T17:03:18.715+0200    info    extensions/extensions.go:48     Extension started.      {"kind": "extension", "name": "file_storage"}
2023-04-06T17:03:18.715+0200    info    adapter/receiver.go:56  Starting stanza receiver        {"kind": "receiver", "name": "filelog/logs/json", "data_type": "logs"}
2023-04-06T17:03:18.715+0200    info    service/service.go:155  Starting shutdown...
2023-04-06T17:03:18.715+0200    info    adapter/receiver.go:150 Stopping stanza receiver        {"kind": "receiver", "name": "filelog/logs/json", "data_type": "logs"}
2023-04-06T17:03:18.715+0200    info    extensions/extensions.go:55     Stopping extensions...
2023-04-06T17:03:18.715+0200    info    service/service.go:169  Shutdown complete.
Error: cannot start pipelines: storage client: open receiver_filelog_logs/json: no such file or directory
2023/04/06 17:03:18 collector server run finished with error: cannot start pipelines: storage client: open receiver_filelog_logs/json: no such file or directory

I agree this should be fixed by sanitizing the receiver's name by replacing the slashes with another character. How about the underscore _? It is already being used as the separator between the receiver type filelog and the instnce name.

@andrzej-stencel andrzej-stencel added extension/storage/filestorage and removed needs triage New item requiring triage labels Apr 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2023

Pinging code owners for extension/storage/filestorage: @djaglowski. See Adding Labels via Comments if you do not have permissions to add labels yourself.

@newly12
Copy link
Contributor Author

newly12 commented Apr 6, 2023

Thanks for looking into this. I don't have any concern to use underscore.

@djaglowski
Copy link
Member

I think we should use a character that is less likely to be one chosen by a user, because for example we would still have a collision between a/b_c and a_b/c. @newly12 suggested ~ which seems like a better choice to me.

@djaglowski
Copy link
Member

Related issue: #3148

@andrzej-stencel
Copy link
Member

@djaglowski would you be OK with a PR that just replaces slashes / with tildes ~ or do you think we should take a broader approach?

@djaglowski
Copy link
Member

@astencel-sumo, that would be an improvement on what we have now, so seems fine. I think we should leave the issue open though, until we're confident that the solution is robust.

@andrzej-stencel
Copy link
Member

Here's the PR:

I propose to close this issue with this PR as it is a (sub)duplicate of #3148 that's only about slashes, but keep #3148 open. What do you think @djaglowski?

@andrzej-stencel andrzej-stencel self-assigned this Apr 13, 2023
@atoulme atoulme changed the title [filestroage] receiver name sanitization [filestorage] receiver name sanitization Apr 13, 2023
@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@andrzej-stencel
Copy link
Member

I have reworked the PR mentioned above to fix the more general issue #3148.

@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Aug 29, 2023
@djaglowski djaglowski removed the Stale label Aug 29, 2023
djaglowski pushed a commit that referenced this issue Oct 3, 2023
…ames (#20896)

Fixes #3148

This change was initially created to only fix #20731 by replacing the
slash `/` characters in component names with a tilde `~`. After that, we
decided that it is a breaking change, and so it's better to fix other
characters as well in a single breaking change.
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this issue Nov 12, 2023
…ames (open-telemetry#20896)

Fixes open-telemetry#3148

This change was initially created to only fix open-telemetry#20731 by replacing the
slash `/` characters in component names with a tilde `~`. After that, we
decided that it is a breaking change, and so it's better to fix other
characters as well in a single breaking change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants