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

Adding engine definition under resources semantic conventions #1293

Merged
merged 26 commits into from
Apr 1, 2021
Merged

Adding engine definition under resources semantic conventions #1293

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
1b533b1
Added engine definition
Dec 16, 2020
aed7df8
Engine added
Dec 16, 2020
53a8b08
Added reference to engine.md under Compute Unit
Dec 16, 2020
906839f
Apply suggestions from code review
Dec 29, 2020
679fe46
Update engine.md
Jan 29, 2021
7e78b31
Update engine.md
Jan 29, 2021
69d3da1
Update engine.md
Jan 29, 2021
775dc3e
Update engine.md
Jan 29, 2021
e8ec313
Update engine.md
Jan 29, 2021
0988790
Update engine.md
Jan 29, 2021
ed5957b
Update engine.md
Feb 1, 2021
ef64581
Update engine.md
Feb 1, 2021
4c770dc
Update engine.yaml
Feb 1, 2021
2f95967
Update engine.md
Feb 1, 2021
81fac67
Update engine.md
Feb 1, 2021
4a6f9ef
Apply suggestions from code review
Feb 4, 2021
b70a0ad
Update engine.md
Feb 4, 2021
f78ec33
Update specification/resource/semantic_conventions/engine.md
Feb 5, 2021
15638fa
Merge branch 'main' into engine-definition
carlosalberto Feb 23, 2021
6854721
Merge branch 'main' into engine-definition
Mar 31, 2021
cba1f77
Renamed engine to webengine
Mar 31, 2021
f02e289
Updated reference to webengine
Mar 31, 2021
af1c8f5
Converted to webengine
Mar 31, 2021
0de3728
Rename engine.md to webengine.md
Mar 31, 2021
8691efa
Update webengine.md
Mar 31, 2021
39cd9e0
Merge branch 'main' into engine-definition
carlosalberto Apr 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions semantic_conventions/resource/engine.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
groups:
- id: engine_resource
prefix: engine
brief: >
Resource describing the packaged software running the application code. Engines are executed using process.runtime.
ivomagi marked this conversation as resolved.
Show resolved Hide resolved
attributes:
- id: name
type: string
required: always
brief: >
The name of the engine.
examples: ['WildFly']
- id: version
type: string
brief: >
The version of the engine, as returned by the engine (preferably during runtime) without modification.
ivomagi marked this conversation as resolved.
Show resolved Hide resolved
examples: ['21.0.0']
- id: description
type: string
brief: >
Additional description about the engine.
ivomagi marked this conversation as resolved.
Show resolved Hide resolved
examples: ['WildFly Full 21.0.0.Final (WildFly Core 13.0.1.Final) - 2.2.2.Final']
1 change: 1 addition & 0 deletions specification/resource/semantic_conventions/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ Attributes defining a compute unit (e.g. Container, Process, Function as a Servi
- [Container](./container.md)
- [Function as a Service](./faas.md)
- [Process](./process.md)
- [Engine](./engine.md)

## Compute Instance

Expand Down
22 changes: 22 additions & 0 deletions specification/resource/semantic_conventions/engine.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Engine

**type:** `engine`

**Description:** Resource describing the packaged software running the application code. Engines are typically executed using process.runtime.
Copy link
Member

Choose a reason for hiding this comment

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

Engines are typically executed using process.runtime.

I don't understand what this means.

Copy link
Member

Choose a reason for hiding this comment

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

For example, WildFly is executed using a JVM.

Copy link
Member

@yurishkuro yurishkuro Feb 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I understand what an "engine" is (from examples), I am referring to the wording "using process.runtime". Is it referring to the resource attribute?

Copy link
Member

Choose a reason for hiding this comment

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

in other words, I don't think this wording is helping or explaining it well.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think so.

Copy link
Member

Choose a reason for hiding this comment

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

there are already sub-sections that make sense for specific languages, like process.runtime.dotnet-core, or process.runtime.nodejs

There is no process.runtime.nodejs semantic convention. These subsection are just very outdated (maybe we should bug @open-telemetry/javascript-approvers and @open-telemetry/dotnet-approvers to update them)? What is actually meant is that you should one of the values in that table as process.runtime.name. E.g. you would use process.runtime.name=browser (and telemetry.sdk.language=webjs) to indicate Javascript in the browser.

Copy link
Contributor

Choose a reason for hiding this comment

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

almost none of the other SIGs responded saying that it makes sense to them

My take on that is that none of other SIGs objected to this.

I don't see where any consensus was reached

I thought that approval means agreement. Am I wrong?

@yurishkuro And I beg you pardon, but you haven't answered my question: do you want to block this PR from merging?

Copy link
Member

Choose a reason for hiding this comment

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

I am not in the habit of exercising veto power. I have voiced my concerns with the wording.

Copy link
Contributor

Choose a reason for hiding this comment

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

So you actually don't care what will happen with this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having a conversation about naming is important, semantic conventions are about naming in the end of the day. "Engine" might not be the best name in this case. Is it standing for a runtime, a platform, an execution environment? It matters to have this conversation upfront because the naming sets the scope and sometimes affects the other specs. For example, if "engine" stands for a platform/runtime, we have to iterate on platform specific specs like Lambda, etc.


<!-- semconv engine_resource -->
| Attribute | Type | Description | Examples | Required |
|---|---|---|---|---|
| `engine.name` | string | The name of the engine. | `WildFly` | Yes |
| `engine.version` | string | The version of the engine, as returned by the engine (preferably during runtime) without modification. | `21.0.0` | No |
| `engine.description` | string | Additional description about the engine. | `WildFly Full 21.0.0.Final (WildFly Core 13.0.1.Final) - 2.2.2.Final` | No |
<!-- endsemconv -->

Information describing the engine SHOULD be captured using the values acquired from the API provided by the engine, preferably during runtime, to avoid maintenance burden on engine version upgrades. As an example - Java engines are often but not always packaged as application servers. For Java application servers supporting Servlet API the required information MAY be captured by invoking `ServletContext.getServerInfo()` during runtime and parsing the result.
iNikem marked this conversation as resolved.
Show resolved Hide resolved

A resource can be attributed to at most one engine.

The situations where there are multiple candidates, it is up to instrumentation library authors to choose the engine. To illustrate, let's look at a Python application using Apache HTTP Server with mod_wsgi as the server and Django as the web framework. In this situation:
Copy link
Member

Choose a reason for hiding this comment

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

it is up to instrumentation library authors to choose the engine

This is incorrect. In the example below the "instrumentation library" will be only instrumenting Django framework, regardless of where it is run standalone or via wsgi. The "enging" is a part of the resource definition, so only the SDK can populate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an ongoing discussion at #1298 in order to allow resources to be added post-creation. But indeed, up until this is allowed, you are correct. It seems we have a dependency for that matter and I agree to wait with the merge until the #1298 gets a resolution.


* Either Apache HTTP Server or `mod_wsgi` MAY be chosen as `engine`, depending on the decision made by the instrumentation authors.
Copy link
Member

Choose a reason for hiding this comment

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

According to the definition above, neither can be used as engine, since the runtime will be Python and neither Apache nor mod_wsgi are executed using Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relaxed the definition to cover for this (Engines are typically executed using process.runtime).

* Django would SHOULD NOT be set as an engine as the required information is already available in insrumentation library and setting this into `engine` would duplicate the information.
ivomagi marked this conversation as resolved.
Show resolved Hide resolved