Remove rpc.jsonrpc.method, clarify rpc.method instead.

[Oberon00]

(no corresponding issue)

Changes

Remove rpc.jsonrpc.method, clarify that rpc.method should be used instead.

Related

• PR that introduced JSON RPC conventions: added JSON RPC specific attributes #1643 (@Rast1234)

This change was just released in 1.4, but semantic conventions are still experimental thus a breaking change like this may be acceptable.

[Rast1234]

So you changed the purpose of rpc.method to be logical, not physical method/function name on the server? Great!

[Oberon00]

So you changed the purpose of rpc.method to be logical, not physical method/function name on the server?

In my mind, this was already the case before, but the wording was not very clear.

[carlosalberto]

@Rast1234 Just to double confirm: are you ok with this change?

[Rast1234]

Yes!

[arminru]

Thanks! I think explicitly declaring this as the logical names and referencing the code.* attributes makes sense in any case, not only for JSON-RPC.

[carlosalberto]

LGTM once the feedback has been addressed.

[carlosalberto]

@Oberon00 Please update your PR so we can merge it.

[jkwatson]

This should include an update to the schema, correct? Our schema does support re-writing attributes from old to new, I believe.

[Oberon00]

@jkwatson

This should include an update to the schema, correct? Our schema does support re-writing attributes from old to new, I believe.

Hmm, probably, unless we consider schema updates part of the work of cutting a release and do it all at once at the end.
In that case we rename an attribute to an already existing attribute. I'm not sure what should happen if the existing rpc.method attribute is already filled out. Based on what I think was the misunderstanding about rpc.method that caused rpc.jsonrpc.method, I think that rpc.method should be overwritten with rpc.jsonrpc.method.

That being said, I would appreciate help with writing the schema, as there are no examples to draw from yet? @tigrannajaryan, can you assist?

[carlosalberto]

Ping @tigrannajaryan

[tigrannajaryan]

Assuming this is really semantically an attribute rename this schema file should do it:

file_format: 1.0.0
schema_url: https://opentelemetry.io/schemas/1.5.0
versions:
  1.5.0:
    spans:
      changes:
        # Sequence of translations to apply to convert the schema from a prior version
        # to this version. The order in this sequence is important. Translations are
        # applied from top to bottom in the listed order.
        - rename_attributes:
            # Rename attributes of all spans, regardless of span name.
            # The keys are the old attribute name used prior to this version, the values are
            # the new attribute name starting from this version.
            attribute_map:
              rpc.jsonrpc.method: rpc.method
  1.4.0:

The file should be named schemas/1.5.0 (no extension in the file name).

Note, I did not review the PR thoroughly, so I am not completely sure it is actually a renaming and not a deletion of an old attribute and introduction of a new unrelated attribute.

P.S. I have put together a small tool to do some sanity checks of schema files: https://github.com/tigrannajaryan/telemetry-schema/blob/main/cmd/scheck/main.go

[Oberon00]

@tigrannajaryan

Note, I did not review the PR thoroughly, so I am not completely sure it is actually a renaming and not a deletion of an old attribute and introduction of a new unrelated attribute.

This PR is a deletion of a new attribute because it can already by represented by an existing attribute (new -> existing). So it's not exactly a renaming (new1 -> new2).
What would rename_attributes do if both the new and old attribute are set?

[tigrannajaryan]

What would rename_attributes do if both the new and old attribute are set?

The behavior is undefined in the OTEP right now. We can define it but I would say if there is an expectation that both may be set then it does not sound like renaming and seems to be something more complicated. I don't think rename_attributes can handle that.

[Oberon00]

So then is it fine to merge this PR without a schema entry?

[tigrannajaryan]

So then is it fine to merge this PR without a schema entry?

Yes, I think so. Since the semconv is not yet stable it is fine. In the future if something similar to this happens we will need to figure out how we can describe the change in the schema file.