Opentelemetry-java: Labels with duplicate key must take the last value

[thisthat]

Describe the bug
According to the spec, labels with a duplicated key must take the last value but in the current implementation, the first one is taken.

Steps to reproduce
https://github.com/open-telemetry/opentelemetry-java/blob/a4cd21ff49f7de495c031fac469a367311f7636a/api/src/test/java/io/opentelemetry/common/LabelsTest.java#L77-L86

What did you expect to see?

  @Test
  void deduplication() {
    Labels one =
        Labels.of(
            "key1", "value1",
            "key1", "valueX");
    Labels two = Labels.of("key1", "valueX");

    assertThat(one).isEqualTo(two);
  }

[thisthat]

Since also attributes should have the same behavior (spec link), I think we should change the implementation of dedupe(Object[]) https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/common/ImmutableKeyValuePairs.java#L101 inverting the for loop order.

[Oberon00]

Does this mean #1607 was valid after all?

[jkwatson]

I think that #1607 was still invalid, as things currently stand, but our general de-duplication is probably not correct as it is. So, if we change the core attribute/label implementation, it will break the desired behavior that was discussed in #1607. So, whoever picks this up should be aware that there is a bit of subtlety in implementing this, to make sure that the attribute issues with the recordException method are accounted for.

[jkwatson]

Since also attributes should have the same behavior (spec link), I think we should change the implementation of dedupe(Object[]) https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/common/ImmutableKeyValuePairs.java#L101 inverting the for loop order.

It's not quite that simple, because that will reverse the order of the contents of the final attributes, as well. The implication of this is that forEach needs to also iterate in reverse order to preserve existing functionality. Or, we'd need to reverse the resulting list after du-duping, I suppose [Also, not as simple as Collections.reverse(), since the pairs would then be in reverse order].

[jkwatson]

@Oberon00 Incidentally, the same ends up being true about Resource merging. The implementation was relying on the incorrect implementation of attribute precedence.