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

Add CloudFoundry Resource Attributes #624

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

KarstenSchnitter
Copy link

@KarstenSchnitter KarstenSchnitter commented Dec 29, 2023

Proposal for #622:

Using the CloudFoundry documentation, provides a set of resource attribute describing CloudFoundry workloads. Both applications as well as system components are addressed. Helpful resources:

Changes

Introduces a resource attributes convention for CloudFoundry. Defines resource attributes to identify CloudFoundry applications and system components, enabling and improving OpenTelemetry support. The proposed attributes are selected to provide an as small as possible payload. The changes follow the proposal in #622.

Merge requirement checklist

@KarstenSchnitter KarstenSchnitter requested review from a team December 29, 2023 13:00
Copy link

linux-foundation-easycla bot commented Dec 29, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@KarstenSchnitter
Copy link
Author

This PR is meant as a proposal for discussion around #622. I am unsure about when and how to fulfil the merge requirements checklist. A changelog entry seems to be necessary. I do not know, how to verify the contribution guidelines. Locally a make check ran without issues.

@joaopgrassi
Copy link
Member

@KarstenSchnitter

A changelog entry seems to be necessary

Yes, please add an entry to the CHANGELOG.md file. You can see an example here: #615

I do not know, how to verify the contribution guidelines. Locally a make check ran without issues.

I approved the CI run and some checks are failing. Not sure how the make check passed, but you can run them separately and they for sure should also fail locally.

@KarstenSchnitter
Copy link
Author

@joaopgrassi I will take care of the issues and the changelog. Is there anything else?

@KarstenSchnitter
Copy link
Author

@joaopgrassi I noticed some unmerged changes on my side. Hopefully, this resolves the checks.

@joaopgrassi
Copy link
Member

I only looked at a high level yesterday, and I'm not experienced with CloudFoundry, so I will need to read a bit to understand the context so I can review it. I will try to do it in the upcoming days.

@KarstenSchnitter KarstenSchnitter force-pushed the cf-resource branch 2 times, most recently from 2ee4884 to 9fb2ddc Compare January 11, 2024 09:00
Copy link

@FWinkler79 FWinkler79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should review the naming of some of the attributes.
The OTEL colleagues should weigh in here, as they probably know best about the usual naming conventions.

docs/attributes-registry/cloudfoundry.md Outdated Show resolved Hide resolved
docs/attributes-registry/cloudfoundry.md Outdated Show resolved Hide resolved
docs/resource/cloudfoundry.md Outdated Show resolved Hide resolved
docs/resource/cloudfoundry.md Outdated Show resolved Hide resolved
docs/resource/cloudfoundry.md Outdated Show resolved Hide resolved
@chombium
Copy link

chombium commented Jan 12, 2024

Hi @FWinkler79,
thanks for the tips and the review. I'm one of the maintainers in the Cloud Foundry's logging and metrics team. We've discussed with @KarstenSchnitter and @ctlong and we also had the same doubts about the source_id, source_type and instance_id labels. We've added them as they are currently used in the Cloud Foundry's logging and metrics stack, but after checking the existing conventions, I've found the attributes-registry/source.md and general/attributes.md and I think that we should change them to source.id, source.type and instance.id.

There is one more open question to clear: In Cloud Foundry the source type label is only used for logs and not for metrics and traces. We are not sure what do do about it and if we need to define it in some specific way. @joaopgrassi Any ideas about this?

Using the CloudFoundry documentation, provides a set of
resource attribute describing CloudFoundry workloads. Both
applications as well as system components are addressed.
Helpful resources:

* https://github.com/cloudfoundry/loggregator-api#v2-envelope
* https://docs.cloudfoundry.org/devguide/deploy-apps/environment-variable.html#VCAP-APPLICATION
* https://bosh.io/docs/jobs/#properties-spec
* https://docs.cloudfoundry.org/devguide/deploy-apps/streaming-logs.html

Signed-off-by: Karsten Schnitter <[email protected]>
Signed-off-by: Karsten Schnitter <[email protected]>
* source_id -> source.id
* instance_id -> instance.id
* source_type -> source.type

Add clarification on source_type:
* required if log message
* comment on obtaining the value

Signed-off-by: Karsten Schnitter <[email protected]>
@KarstenSchnitter
Copy link
Author

I provided a revised version with the fields renamed as suggested by @FWinkler79. Furthermore, I added some clarification on the source_type, especially the requirement level. For the three use-cases in #622 I added a comment, how to obtain the appropriate value for use-case 1, where the value needs to be set to a static value. This is the reason, why I think source.typeshould be part of this convention, but only apply to logs as now reflected in the requirement level. The branch to be merged was rebased on main in the process.

@FWinkler79
Copy link

FWinkler79 commented Jan 15, 2024

Thanks for your reply and changes.

@chombium:

There is one more open question to clear: In Cloud Foundry the source type label is only used for logs and not for metrics and traces. We are not sure what do do about it and if we need to define it in some specific way. @joaopgrassi Any ideas about this?

I am not 100% sure, but I recently updated our internal version of the OTEL SemConv to the latest version and noticed that the attributes for different signal types are now (rather) cleanly separated into log-attributes, trace-attributes, etc.
Not sure the documentation already properly reflects that, but at least the YAML specifications do.
E.g. you can find the entire yaml-based model files here. There you find folders for logs, metrics, traces and recource attributes.
To avoid data duplication the common "base attributes" that are shared between signal types are now maintained under the "registry" folder. It's not entirely cleaned up yet, but I think that's the intention.

For example, in the registry folder you can find http.yaml, in the trace folder you also find http.yaml. The trace-related HTTP attributes simply reference HTTP attributes defined in the file under registry.

In a similar way, I think you could define log-specific attributes.

@chombium
Copy link

Thanks for the prompt change @KarstenSchnitter and for the suggestions and digging into the definitions @FWinkler79. @joaopgrassi Is there something else to be done apart from the suggestions about the source type parameter?

model/registry/cloudfoundry.yaml Outdated Show resolved Hide resolved
model/registry/cloudfoundry.yaml Outdated Show resolved Hide resolved
model/registry/cloudfoundry.yaml Outdated Show resolved Hide resolved
model/registry/cloudfoundry.yaml Outdated Show resolved Hide resolved
model/registry/cloudfoundry.yaml Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Feb 5, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 5, 2024
@KarstenSchnitter
Copy link
Author

@chombium , @ctlong, can you provide a status of your deliberations? Maybe we can agree on an initial convention and improve over time?

@joaopgrassi
Copy link
Member

Hi @KarstenSchnitter !

We changed how the CHANGELOG.md is managed. Please take a look at https://github.com/open-telemetry/semantic-conventions/blob/main/CONTRIBUTING.md#adding-a-changelog-entry to see what needs to be done. Sorry for the disruption.

@github-actions github-actions bot removed the Stale label Feb 6, 2024
@FWinkler79
Copy link

@KarstenSchnitter I think you still wanted to add something here, right?

@KarstenSchnitter
Copy link
Author

I think, I have addressed all open questions, but need some feedback from @lmolkova before marking them as resolved. If the current state is acceptable, there is nothing to be changed from my side.

@github-actions github-actions bot removed the Stale label Jul 25, 2024
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 23, 2024
@KarstenSchnitter
Copy link
Author

@joaopgrassi and @lmolkova is there a way we can make progress here? The PR is approved by subject matter experts from the Cloud Foundry community.

@github-actions github-actions bot removed the Stale label Aug 24, 2024
@joaopgrassi
Copy link
Member

I will try and take a look at it again in the upcoming days.

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at the checks failing. Also update the issue templates by running make generate-gh-issue-templates and make fix to update any left over tables.

One general ask I have to the group is: Resource attributes must be immutable - that means a resource is created during the app start up with say CloudFoundry attributes. Once the app is up and running, these attributes cannot change. I wanted to make this clear so you as the experts do the exercise in seeing if there's any such attributes introduced here that are affected by this constraint.

Another fact is that there's effort currently in a new group for Resource and Entities. The reason I'm bringing it up is since this PR is adding resource attributes I'm not sure if we want to block it. Maybe we allow it to be part of the registry as a first step.

@jsuereth could you please share your thoughts here?

model/registry/cloudfoundry.yaml Outdated Show resolved Hide resolved
result of running
`make generate-gh-issue-templates`

Signed-off-by: Karsten Schnitter <[email protected]>
Adjust model registry for CloudFoundry to not use
prefixes but fully qualified ids.

Signed-off-by: Karsten Schnitter <[email protected]>
Avoids misspelling of CloudFoundry during content
generation.

Signed-off-by: Karsten Schnitter <[email protected]>
Regenerated templates for proper order.

Signed-off-by: Karsten Schnitter <[email protected]>
Removes prefixes from CloudFoundry groups.

Signed-off-by: Karsten Schnitter <[email protected]>
Copy link
Author

@KarstenSchnitter KarstenSchnitter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took care of the prefixes and hopefully all validation errors. I can confirm, that all resource attributes are immutable. Once an application instance is started, all its processes will have uniquely and immutably defined resource attributes of the proposed set.

model/registry/cloudfoundry.yaml Outdated Show resolved Hide resolved
Moves resource and registry yaml to cloudfoundry directory.
Add attribute names.

Signed-off-by: Karsten Schnitter <[email protected]>
result of `make generate-gh-issue-templates`

Signed-off-by: Karsten Schnitter <[email protected]>
@KarstenSchnitter
Copy link
Author

There is an issue on the main branch with the Github link to the registry, which has a 404 error:

https://github.com/open-telemetry/semantic-conventions/blob/b855a36cf4b8a3c5461ad16fa4ace0c615d11c2a/CONTRIBUTING.md?plain=1#L225C15-L225C94

This fails the tests, but is not caused by this PR.

@KarstenSchnitter
Copy link
Author

The latest merge should fix the issue with the main branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Discussions
Development

Successfully merging this pull request may close these issues.

8 participants