open-telemetry / open-telemetry/opentelemetry-java

Feedback on `ExtendedAttributes` API

Open
#7,323 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Bug
Dominant language
Java
Stars
2.5k
Forks
1k
Avg merge
3d 17h
Merged PRs (30d)
58

Description

Hi @jack-berg, thanks for working on #7123. As discussed below the PR, I'll drop here some feedback I collected while working on open-telemetry/opentelemetry-java-instrumentation#8354. There may be more, I'll update the issue if I think about something else.

InternalExtendedAttributeKeyImpl#toString should contain type information

The current implementation makes inspecting test failures quite confusing:

    Expecting map:
      {string1="1", string2="2"}
    to contain entries:
      [map={string1="1", string2="2"}]
    but could not find the following map entries:
      [map={string1="1", string2="2"}]

I think it would make sense to embed the additional information in the string.

ExtendedAttributesBuilder#put(String, Object)

Sometimes the type information at compile time is erased due to attributes being pushed into a Map<String, Object> (e.g. Log4j's ThreadContext#getThreadContextMap). Thus, we have to write something like this:

    if (value instanceof String) {
      attributes.put(
          (ExtendedAttributeKey<String>) keyProvider.apply(key, ExtendedAttributeType.STRING),
          (String) value);
    } else if (value instanceof Boolean) {
      attributes.put(
          (ExtendedAttributeKey<Boolean>) keyProvider.apply(key, ExtendedAttributeType.BOOLEAN),
          (Boolean) value);
[...]

This same logic might be duplicated in many different places, increasing the likelihood of mistakes. Example here. Is there a better way to do it in the current implementation?

It would be nice to have a public utility to expose InternalExtendedAttributeKeyImpl.create

Sometimes it's useful to use this factory directly, e.g. here. Should I consider it public API? I guess not.

It would be nice to extend the assertion framework for ExtendedAttributes

Writing tests for ExtendedAttributes is possible at the moment, but a bit cumbersome. I guess LogRecordDataAssert will be updated only when the API stabilizes, right?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with InternalExtendedAttributeKeyImpl#toString/create, ExtendedAttributesBuilder#put(String, Object), and LogRecordDataAssert, then read the surrounding implementation and tests. The issue contains four separate proposals, so first select one and define its expected API or assertion behavior; completion is not currently described by a single criterion.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
api, backend-api-design, testing
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
28/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.