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.

Sign up for GitHub

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

Jump to bottom

[processor/resourcedetection] Additional system attributes #22045

Closed
4 tasks done
mx-psi opened this issue May 17, 2023 · 7 comments · Fixed by #29588
Closed
4 tasks done

[processor/resourcedetection] Additional system attributes #22045

mx-psi opened this issue May 17, 2023 · 7 comments · Fixed by #29588
Labels
enhancement New feature or request priority:p2 Medium processor/resourcedetection Resource detection processor

Comments

@mx-psi
Copy link
Member

mx-psi commented May 17, 2023 •
edited
Loading

Component(s)

processor/resourcedetection

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

I would like to add some additional attributes to be fetched by the system detector. These would provide useful context on the machine that is generating telemetry and would be useful to my vendor for infrastructure monitoring. All of the following are in one of the specs (ECS or OTel's). I will open separate issues for other attributes not in the spec.

Describe the solution you'd like

Add the following host or os attributes:

Some of these are available in Elastic Common Schema, I will work on the semantic conventions repository to add these before putting them in the system detector.

Describe alternatives you've considered

We could add these to a different processor or detector, but the system detector seems like the best place to put these.

Additional context

As far as I can tell, no other detector supports these, so there should be no problems similar to #21233, so we can safely add these by default.
@mx-psi mx-psi added enhancement New feature or request priority:p2 Medium processor/resourcedetection Resource detection processor labels May 17, 2023
@github-actions
Copy link
Contributor

Pinging code owners for processor/resourcedetection: @Aneurysm9 @dashpole. See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions
Copy link
Contributor

Pinging code owners:

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

@dmitryax
Copy link
Member

@mx-psi can we wait for #21482 to land before this? I believe most of the new fields are better to be added as optional.

@dmitryax dmitryax mentioned this issue May 30, 2023
Merged
@mx-psi
Copy link
Member Author

mx-psi commented May 31, 2023

@mx-psi can we wait for #21482 to land before this? I believe most of the new fields are better to be added as optional.

I don't think these attributes suffer from the same issues as host.name or host.id: for the most part there is single well defined value that they should have, so while I think it is useful to be able to enable/disable them, I don't think #21482 should block this. Do you have a specific example on what other component/SDK detector these would conflict with?

I am also undecided on whether they should be optional or not; I am not against making them optional but I think we should understand why we want to make them optional (specially considering the precedent on the resource detection processor which has historically been "just add everything as enabled by default")

@dmitryax
Copy link
Member

dmitryax commented May 31, 2023 •
edited
Loading

Enabling all the attributes significantly increases the payload for the users that enable the system detector, and not all of them seem to be essential as host.name or host.id. Given that it's a big list, I'd suggest we evaluate them and define optional/default now instead of changing them later.

Also, I think all of them should be defined in the spec. Anything that is not defined there yet, should be optional to avoid future breaking changes in future. Once it's there, we can align default/optional with the spec's required/recommended.

@mx-psi
Copy link
Member Author

mx-psi commented May 31, 2023

Enabling all the attributes significantly increases the payload for the users that enable the system detector, and not all of them seem to be essential as host.name or host.id. Given that it's a big list, I'd suggest we evaluate them and define optional/default now instead of changing them later.

I agree in the abstract but I am not sure what criteria we should use for this. One possibility is to use the 'Requirement Level' as a guide for this, so, e.g., os.type should be enabled by default since it is 'Required' but any other os.* attribute should be disabled by default. However, both host.name and host.id are only 'recommended', while to me it seems obvious that we should keep them as enabled. Any thoughts on what criteria to use here?

Also, I think all of them should be defined in the spec. Anything that is not defined there yet, should be optional to avoid future breaking changes in future. Once it's there, we can align default/optional with the spec's required/recommended

That's a good point. All the conventions listed here are either on the OTel spec or on the ECS spec, but I guess since they have not been formally merged, we should make those that come from ECS optional for now 👍 I also want to add some others that are not on either spec, so it will be useful for those as well.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

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 Oct 4, 2023
@mx-psi mx-psi removed the Stale label Oct 4, 2023
@mx-psi mx-psi mentioned this issue Nov 30, 2023
Merged
mx-psi added a commit that referenced this issue Dec 7, 2023
@mx-psi @crobert-1
[processor/resourcedetection] Add host.mac to system detector (#29588)
cdfaa45 
**Description:**

Adds support for `host.mac` detection to the `system` detector on the
resource detection processor.

This convention is defined in the specification [on the host
document](https://github.com/open-telemetry/semantic-conventions/blob/v1.23.1/docs/resource/host.md).

**Link to tracking Issue:** Fixes #29587 and therefore fixes #22045

**Testing:** 

Unit tests; manually Tested on my laptop with the following
configuration:

```
receivers:
  hostmetrics:
    collection_interval: 10s
    scrapers:
      load:

processors:
  resourcedetection:
    detectors: ["system"]
    system:
      resource_attributes:
        host.mac:
          enabled: true

exporters:
  debug:
    verbosity: detailed

service:
  pipelines:
    metrics:
      receivers: [hostmetrics]
      processors: [resourcedetection]
      exporters: [debug]
```

---------

Co-authored-by: Curtis Robert <[email protected]>
jayasai470 pushed a commit to jayasai470/opentelemetry-collector-contrib that referenced this issue Dec 8, 2023
@mx-psi @crobert-1 @jayasai470
[processor/resourcedetection] Add host.mac to system detector (open-t…
4a89d29 
…elemetry#29588)

**Description:**

Adds support for `host.mac` detection to the `system` detector on the
resource detection processor.

This convention is defined in the specification [on the host
document](https://github.com/open-telemetry/semantic-conventions/blob/v1.23.1/docs/resource/host.md).

**Link to tracking Issue:** Fixes open-telemetry#29587 and therefore fixes open-telemetry#22045

**Testing:** 

Unit tests; manually Tested on my laptop with the following
configuration:

```
receivers:
  hostmetrics:
    collection_interval: 10s
    scrapers:
      load:

processors:
  resourcedetection:
    detectors: ["system"]
    system:
      resource_attributes:
        host.mac:
          enabled: true

exporters:
  debug:
    verbosity: detailed

service:
  pipelines:
    metrics:
      receivers: [hostmetrics]
      processors: [resourcedetection]
      exporters: [debug]
```

---------

Co-authored-by: Curtis Robert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request priority:p2 Medium processor/resourcedetection Resource detection processor
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[processor/resourcedetection] Add host.mac to system detector mx-psi/opentelemetry-collector-contrib
2 participants
@mx-psi @dmitryax