-
Notifications
You must be signed in to change notification settings - Fork 273
Update Heroku Semantic Conventions to Match Official Documentation #2843
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
base: main
Are you sure you want to change the base?
Update Heroku Semantic Conventions to Match Official Documentation #2843
Conversation
Update Heroku attributes to reflect only officially documented attributes from Heroku OpenTelemetry Signals and Attributes Reference.
|
- id: heroku.release.creation_timestamp | ||
# Core Application Attributes | ||
- id: heroku.app.id | ||
type: string | ||
stability: development | ||
stability: stable | ||
brief: > | ||
Time and date the release was created | ||
examples: [ '2022-10-23T18:00:42Z' ] | ||
- id: heroku.release.commit | ||
The unique identifier of the Heroku application. | ||
This is a UUID that uniquely identifies the application across the Heroku platform. | ||
examples: [ '9daa2797-e49b-4624-932f-ec3f9688e3da', 'c3d3df33-8afb-4323-ac49-a9bf41a50dd1' ] | ||
note: > | ||
This attribute is automatically provided by Heroku and cannot be customized. | ||
- id: heroku.app.name | ||
type: string | ||
stability: development | ||
stability: stable | ||
brief: > | ||
Commit hash for the current release | ||
examples: [ 'e6134959463efd8966b20e75b913cafe3f5ec' ] |
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.
By convention we don't remove attributes in semantic conventions but instead we deprecate them, so could you please add the deprecation object/properties to the old object rather than removing them.
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.
Added deprecated section!
name: heroku | ||
brief: > | ||
[Heroku dyno metadata](https://devcenter.heroku.com/articles/dyno-metadata) | ||
Heroku Fir Application Attributes as defined in the | ||
[Heroku OpenTelemetry Signals and Attributes Reference](https://devcenter.heroku.com/articles/heroku-opentelemetry-signals-and-attributes-reference) | ||
attributes: | ||
- ref: heroku.release.creation_timestamp | ||
requirement_level: opt_in | ||
- ref: heroku.release.commit | ||
requirement_level: opt_in | ||
- ref: heroku.app.id | ||
requirement_level: opt_in | ||
- ref: heroku.app.name | ||
- ref: heroku.workload.id | ||
- ref: heroku.release.id | ||
- ref: heroku.release.version |
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.
Could you have an attempt at setting the role of the attributes as per https://opentelemetry.io/docs/specs/semconv/how-to-write-conventions/resource-and-entities/#how-to-define-identifying-attributes.
My thought would be the entity should actually be heroku.app with heroku.release.id & heroku.app.id as identifying with others being descriptive.
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.
That makes sense! I've added roles to both attributes
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.
Thanks, upon looking closer at the description of the attributes I am now having second thoughts about this entity especially after reading https://devcenter.heroku.com/articles/releases
What I now see is that we should have a heroku.app
& a heroku.release
entity this way when changing the config only a new release entity is created, the identifying would be the corresponding *.id attribute being the identifying attributes.
model/heroku/entities.yaml
Outdated
- ref: heroku.app.id | ||
requirement_level: opt_in | ||
- ref: heroku.app.name | ||
- ref: heroku.workload.id |
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.
So an entity represents an object of interest associated with produced telemetry: traces, metrics, logs, profiles etc.
Based on that a heroku workload appears that it should be defined as a seperate object/entity.
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.
Workload id is more of a process type of the application emitting the signals. We have "web", "worker", "scheduler" as possible examples.
We do have other services / entities emitting signals like api, router or add on services. We can add them to the documentation or keep it application specific if it makes sense.
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.
Ok based on that and https://devcenter.heroku.com/articles/procfile heroku.process.process_type
seems more accurate but I assume renaming it is not an option?
I would recommend removing heroku.workload.id
from the heroku.app
entity and instead placing it on a new heroku.process
entity.
Reason for this is, my understanding is that an app can have multiple process types with each process type running in a seperate process. By splitting it we can have a common long lived heroku.app
entity for all signals and then a short lived heroku.process
entity for the process. The heroku.process
would be an extension to the process
entity as described in https://opentelemetry.io/docs/specs/semconv/how-to-write-conventions/resource-and-entities/#extending-an-entity
… roles to app.id and release.id attributes
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.
Reason for blocking pr has been addressed. I do however feel that the entity needs more refinement with comments left.
Also a documentation generation needs to be run to resolve other concerns.
model/heroku/entities.yaml
Outdated
- ref: heroku.app.name | ||
- ref: heroku.workload.id |
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.
These should have the descriptive role.
name: heroku | ||
brief: > | ||
[Heroku dyno metadata](https://devcenter.heroku.com/articles/dyno-metadata) | ||
Heroku Fir Application Attributes as defined in the | ||
[Heroku OpenTelemetry Signals and Attributes Reference](https://devcenter.heroku.com/articles/heroku-opentelemetry-signals-and-attributes-reference) | ||
attributes: | ||
- ref: heroku.release.creation_timestamp | ||
requirement_level: opt_in | ||
- ref: heroku.release.commit | ||
requirement_level: opt_in | ||
- ref: heroku.app.id | ||
requirement_level: opt_in | ||
- ref: heroku.app.name | ||
- ref: heroku.workload.id | ||
- ref: heroku.release.id | ||
- ref: heroku.release.version |
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.
Thanks, upon looking closer at the description of the attributes I am now having second thoughts about this entity especially after reading https://devcenter.heroku.com/articles/releases
What I now see is that we should have a heroku.app
& a heroku.release
entity this way when changing the config only a new release entity is created, the identifying would be the corresponding *.id attribute being the identifying attributes.
model/heroku/entities.yaml
Outdated
- ref: heroku.app.id | ||
requirement_level: opt_in | ||
- ref: heroku.app.name | ||
- ref: heroku.workload.id |
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.
Ok based on that and https://devcenter.heroku.com/articles/procfile heroku.process.process_type
seems more accurate but I assume renaming it is not an option?
I would recommend removing heroku.workload.id
from the heroku.app
entity and instead placing it on a new heroku.process
entity.
Reason for this is, my understanding is that an app can have multiple process types with each process type running in a seperate process. By splitting it we can have a common long lived heroku.app
entity for all signals and then a short lived heroku.process
entity for the process. The heroku.process
would be an extension to the process
entity as described in https://opentelemetry.io/docs/specs/semconv/how-to-write-conventions/resource-and-entities/#extending-an-entity
**Mapping:** | ||
|
||
| Dyno metadata environment variable | Resource attribute | | ||
|------------------------------------|-------------------------------------| | ||
| `HEROKU_APP_ID` | `heroku.app.id` | | ||
| `HEROKU_APP_NAME` | `service.name` | | ||
| `HEROKU_DYNO_ID` | `service.instance.id` | | ||
| `HEROKU_RELEASE_CREATED_AT` | `heroku.release.creation_timestamp` | | ||
| `HEROKU_RELEASE_VERSION` | `service.version` | | ||
| `HEROKU_SLUG_COMMIT` | `heroku.release.commit` | | ||
For FIR applications, service-level attributes can be configured using standard OpenTelemetry environment variables: | ||
|
||
| OTEL Environment Variable | Resource Attribute | Description | | ||
|---------------------------|-------------------|-------------| | ||
| `OTEL_SERVICE_NAME` | `service.name` | Service name (replaces default application name) | | ||
| `OTEL_RESOURCE_ATTRIBUTES` | `service.namespace` | Namespace for service name | | ||
| `OTEL_RESOURCE_ATTRIBUTES` | `service.version` | Version of app release | | ||
| `OTEL_RESOURCE_ATTRIBUTES` | `service.instance.id` | Unique dyno identifier (Heroku-provided) | |
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.
Don't see the need for this given it should be handled via the service documentation.
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.
What if we create an entity "heroku.workload" instead of "heroku.process". It would be hard to rename that attribute. I've added it in my change, lmk if that looks right!
I've seperated them into three entities entity.heroku.release, entity.heroku.app and entity.heroku.workload
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.
Sure that works for me. I would add "extends: process" to the entity definition and then make the workload attribute descriptive.
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.
I was able to add entity.process to workload definition like so:
- id: entity.heroku.workload
type: entity
stability: development
name: heroku.workload
extends: entity.process
brief: >
Heroku Fir Workload Attributes representing individual dyno instances running within a Heroku application.
attributes:
- ref: heroku.workload.id
role: descriptive
But this appends process.*
attributes to the docs/resource/cloud-provider/heroku.md
that we don't provide, does that seem correct?
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 to me as if you deploy 2 copies of a release with the same process type you need a way to distinguish each instance. That can now be done via the process.pid and It also follows what is spoken about in that article.
Changes
The Heroku Telemetry Team is looking to update the current Heroku semantic conventions to match our existing documentation for Fir generation applications. This PR aligns the semantic conventions with the official Heroku OpenTelemetry Signals and Attributes Reference.
What we are changing
Updated Attributes to Match Official Documentation
heroku.dyno_index
,heroku.dyno_type
,heroku.release.creation_timestamp
,heroku.release.commit
heroku.app.id
- unique identifier of appheroku.app.name
- name of applicationheroku.release.id
- unique identifier of app releaseheroku.release.version
- version of app release when telemetry is generatedheroku.workload.id
- workload identifier, as defined by the process typeAdditional Information
OTEL_SERVICE_NAME
,OTEL_RESOURCE_ATTRIBUTES
)(Note: I'm currently going through internal process of getting CLA signed)
Merge requirement checklist
[chore]