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.

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

Remove Attributes combination from onEnd #11959

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

wgy035
Copy link
Contributor

@wgy035 wgy035 commented Aug 7, 2024

Hopefully resolves #11887

Remove reduntant attributes combination in OperationListener.onEnd, move all attributes extract to AttributesExtractor.onEnd

@wgy035 wgy035 requested a review from a team August 7, 2024 08:21
@wgy035 wgy035 changed the title Remove Attributes combanation from onEnd Remove Attributes combination from onEnd Aug 7, 2024
@@ -250,6 +250,7 @@ private void doEnd(
if (operationListeners.length != 0) {
long endNanos = getNanos(endTime);
for (int i = operationListeners.length - 1; i >= 0; i--) {

Copy link
Contributor

Choose a reason for hiding this comment

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

The modification here is useless.

Copy link
Contributor

@steverao steverao left a comment

Choose a reason for hiding this comment

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

There are some CI failures, you can solve them by referring to https://gradle.com/s/7qpfxdz7iymuy

@@ -75,14 +75,7 @@ public static <REQUEST, RESPONSE> HttpServerAttributesExtractorBuilder<REQUEST,

@Override
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
super.onStart(attributes, parentContext, request);
Copy link
Contributor

Choose a reason for hiding this comment

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

Only the attributes that are added at the start will be available for sampling. Moving all the attributes to the onEnd will make sampling impossible.

Copy link
Contributor Author

@wgy035 wgy035 Aug 7, 2024

Choose a reason for hiding this comment

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

So I need to figure out which attributes impact sampling, and those attributes are the minimum set need to retain, right?

According to semantic-conventions(e.g. http-spans), I'd like to extract the unique attributes of span at the start without needing to pass them on. The common attributes attributes should be extracted at the end.

@laurit
Copy link
Contributor

laurit commented Aug 7, 2024

Also see #11092

@laurit
Copy link
Contributor

laurit commented Aug 8, 2024

Did you consider using something like

public final class CompositeAttributes implements Attributes {
  private final Attributes first;
  private final Attributes second;

  private CompositeAttributes(Attributes first, Attributes second) {
    this.first = first;
    this.second = second;
  }

  public static Attributes of(Attributes first, Attributes second) {
    return new CompositeAttributes(first, second);
  }

  @Nullable
  @Override
  public <T> T get(AttributeKey<T> attributeKey) {
    T value = first.get(attributeKey);
    if (value != null) {
      return value;
    }
    return second.get(attributeKey);
  }

  @Override
  public void forEach(BiConsumer<? super AttributeKey<?>, ? super Object> biConsumer) {
    first.forEach(biConsumer);
    second.forEach(biConsumer);
  }

  @Override
  public int size() {
    return first.size() + second.size();
  }

  @Override
  public boolean isEmpty() {
    return first.isEmpty() && second.isEmpty();
  }

  @Override
  public Map<AttributeKey<?>, Object> asMap() {
    Map<AttributeKey<?>, Object> map = new HashMap<>();
    map.putAll(first.asMap());
    map.putAll(second.asMap());
    return map;
  }

  @Override
  public AttributesBuilder toBuilder() {
    return first.toBuilder().putAll(second);
  }
}

@laurit
Copy link
Contributor

laurit commented Aug 16, 2024

@wgy035 have you had a chance to check whether composing the start and end attributes as described above would help?

@wgy035
Copy link
Contributor Author

wgy035 commented Aug 17, 2024

@wgy035 have you had a chance to check whether composing the start and end attributes as described above would help?

@laurit yep, using such a composite structure will reduce traverse overhead and approximately a CompositeAttributesBuilder is also needed, now I'm testing the performance improvement and the code will be updated with a new commit.

@wgy035 wgy035 force-pushed the instrumenter-metrics-optimize branch from e023041 to 41b9c6e Compare September 13, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could we avoid attributes merge in HttpServerMetrics.onEnd?
3 participants