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

Support for embedding of single attributes #332

Closed
wants to merge 3 commits into from
Closed

Support for embedding of single attributes #332

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8633572
update build tools to support embedding of attribute implemented in w…
trisch-me Jul 3, 2024
5416cf0
empty lines removed
trisch-me Jul 3, 2024
2925e35
Merge branch 'main' into embed_attr
trisch-me Jul 16, 2024
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
5 changes: 5 additions & 0 deletions semantic-conventions/semconv.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,11 @@
"tag": {
"type": "string",
"description": "associates a tag to the attribute"
},
"prefix": {
"type": "boolean",
"description": "specifies if it is embedded attribute from other namespace. It defaults to false.",
"default": false
}
}
},
Expand Down
7 changes: 6 additions & 1 deletion semantic-conventions/src/opentelemetry/semconv/model/semantic_attribute.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ def parse(
"requirement_level",
"sampling_relevant",
"note",
"prefix",
)
if not yaml_attributes:
return attributes
Expand Down Expand Up @@ -136,8 +137,12 @@ def parse(
validation_ctx.raise_or_warn(position, msg, ref)
brief = attribute.get("brief")
examples = attribute.get("examples")
ref_prefix = attribute.get("prefix", False)
ref = ref.strip()
fqn = ref
if ref_prefix:
fqn = f"{prefix}.{ref}"
else:
fqn = ref

required_value_map = {
"required": RequirementLevel.REQUIRED,
Expand Down
17 changes: 17 additions & 0 deletions semantic-conventions/src/tests/data/markdown/ref_embed/expected.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Server

<!-- semconv registry.geo(full) -->
| Attribute | Type | Description | Examples | [Requirement Level](https://opentelemetry.io/docs/specs/semconv/general/attribute-requirement-level/) | Stability |
|---|---|---|---|---|---|
| `geo.country` | string | country of the geo coordinates. | `US` | `Recommended` | Experimental |
| `geo.lat` | string | latitude of the geo coordinates. | `123.456` | `Recommended` | Experimental |
| `geo.lon` | string | longitude of the geo coordinates. | `234.456` | `Recommended` | Experimental |
<!-- endsemconv -->


<!-- semconv registry.server(full) -->
| Attribute | Type | Description | Examples | [Requirement Level](https://opentelemetry.io/docs/specs/semconv/general/attribute-requirement-level/) | Stability |
|---|---|---|---|---|---|
| `server.geo.lat` | string | My own latitude | `123.456` | `Recommended` | Experimental |
| `server.id` | string | some server id | `123` | `Recommended` | Experimental |
<!-- endsemconv -->
24 changes: 24 additions & 0 deletions semantic-conventions/src/tests/data/markdown/ref_embed/geo.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
groups:
- id: registry.geo
brief: Attributes describing geo.
type: attribute_group
prefix: geo
attributes:
- id: lat
type: string
stability: experimental
brief: >
latitude of the geo coordinates.
examples: ["123.456"]
- id: lon
type: string
stability: experimental
brief: >
longitude of the geo coordinates.
examples: [ "234.456" ]
- id: country
type: string
stability: experimental
brief: >
country of the geo coordinates.
examples: [ "US" ]
8 changes: 8 additions & 0 deletions semantic-conventions/src/tests/data/markdown/ref_embed/input.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Server

<!-- semconv registry.geo(full) -->
<!-- endsemconv -->


<!-- semconv registry.server(full) -->
<!-- endsemconv -->
15 changes: 15 additions & 0 deletions semantic-conventions/src/tests/data/markdown/ref_embed/server.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
groups:
- id: registry.server
prefix: server
type: attribute_group
brief: "Describes information about the server."
attributes:
- id: id
type: string
stability: experimental
brief: >
some server id
examples: ['123']
- ref: geo.lat
prefix: true
Copy link
Contributor

@lmolkova lmolkova Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe we previously discussed embed: geo.lat instead of ref: geo.lat and prefix: true.

What would be the reason to not do embed: geo.lat based on the original design and do the prefix flag instead?

Why I'd prefer a new verb (embed)

  • ref always means the same thing - reference attribute and keep the original name.
  • obvious when reading. Imagine 10 other lines between ref and prefix and how hard it'd be to understand what name is supposed to be generated
  • easier to check in policies
  • prefix feels like a string property and not a boolean flag.

Copy link
Author

Choose a reason for hiding this comment

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

we have discussed it few weeks ago while I have started to implement it in weaver, so that ref and embed are the same in functionality and difference is only in prefixing the parent namespace or not. Hence this approach was selected. Embed will stay for the whole namespace.

I have announced this change also in embed issue linked in 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.

it still has all the problems listed above, so I'd like to revisit this choice

Copy link
Author

Choose a reason for hiding this comment

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

I can address these problems because IMO they are not critical

  • ref means reference and this is also referencing attribute from another namespace. The difference is in prefix of the attribute. So embed means ref + prefix of the parent group. And this is how it was implemented
  • I personally don't feel this way, we do have a lot of things, like for example ref itself can override description etc. They all means something therefore they are written there
  • It's additional check for policy (ref + prefix) instead just (embed), but I don't feel it's something complicated
  • regarding string property - this is probably due to the fact that we already have such keyword for the group.

The main reason why we went this way was as described before complete similarity between ref and new embed, therefore we have decided to go with it.

I would like to get more opinions on this @jsuereth @lquerel

Copy link
Contributor

@lmolkova lmolkova Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So do you agree that these are problems, critical or not, and ideally we would have one-line that expresses that attribute is embedded instead of two?

It'd also be confusing to have ref to embed one attribute and embed to embed all.

If we want to support both, let's find a good option with embed_one + embed_all, embed + embed_all, etc.

I'm struggling to come up with a real-life use-case for "embed-all-attributes from" and I see a lot of places where embed-one would work great. So both of the above options seem reasonable to me.

Copy link
Author

Choose a reason for hiding this comment

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

we do have already an option to embed all and embed only one. They are written in the original issue. Embeded namespaces will be defined on the top level of the group, while embedded attribute is defined under attributes.

I'm struggling to come up with a real-life use-case for "embed-all-attributes from"

we do have a lot of such real examples. The most prominent one is about geo namespace included into client or server. Then process included into process as parent node and so on.

Similarity in what sense?

In both. On tolling side it has the same implementation with additional prefix. On semantical meaning they also both are references to another namespace with difference in prefix.

Let's discuss it tomorrow during our tooling meeting

Copy link
Contributor

@lmolkova lmolkova Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let's get back to the original concern: having prefix: true under reference has problems (critical or not). We should try to design schema to be easy to use, easy to read, etc. The "not a critical problem" is not a good argument.

Having embed_one and embed_all verbs would result in much better outcome for schema users. So why the pushback?

Copy link
Author

Choose a reason for hiding this comment

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

As I have stated before, this decision was made due to similarity of ref and embed functionality. I don't see this as a problem otherwise I wouldn't go and implement it.
Therefore I propose to discuss it once more during our meeting

Copy link
Contributor

Choose a reason for hiding this comment

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

I do see above problems with this approach, which would be resolved if we used embed_one or similar with no visible downsides.

The embedding is semantically similar to definition of the attribute (as it creates new attribute).

Copy link
Author

Choose a reason for hiding this comment

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

We have discussed it yesterday and I will propose new ideas after a meeting with @lquerel today
Thank you for raising your concerns here.

brief: My own latitude
3 changes: 3 additions & 0 deletions semantic-conventions/src/tests/semconv/templating/test_markdown.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ def testRef(self):
def testRefExtends(self):
self.check("markdown/ref_extends/")

def testRefEmbed(self):
self.check("markdown/ref_embed/")

def testDeprecated(self):
self.check("markdown/deprecated/")

Expand Down
Loading