Opentelemetry-dotnet: Repeated enumeration vs allocation

[alexeyzimarev]

There're several places in the solution where an IEnumerable is repeatedly enumerated.

Some are really weird and seem dangerous, like this one:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry.Api/DistributedContext/DistributedContext.cs#L46

Here the new list is allocated anyway and the whole cumbersome method can be replaced with three lines of code with LINQ. And it will work faster, I tested it, even when the enumerable is a fixed collection and doesn't need to do lazy yield.

Other are justified, like this one:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/e304133b03e1796219f5faba1472565d2108bf11/src/OpenTelemetry/Trace/Span.cs#L103

In this case, the issue is different. The reasoning sounds ok, but you control the options where the IEnumerable<Link> comes from, why keep it as enumerable when you can just use Link[] in the options class and you don't need to make assumptions that it's a small collection? Then, it's up to the client to convert the enumerable.

[lmolkova]

@alexeyzimarev in the first case - there certainly is some room for the optimization - would you mind sending a PR to fix that?

In the second case (links) in many cases they will never be enumerated (sampler may disregard them, and if span is sampled out, they won't be touched again). Assuming there are a few of them (yes, I stay strong on this assumption) you would prefer to re-enumeration over unnecessary allocation (oh, what is there are 0 links like in 99% of cases). If you have a lot of links (like you received 1000 kafka messages), perhaps your have such a network overhead to propagate their contexts that re-enumeration is not a concern. In absence of the scenario and detailed benchmarks it's not possible to tell what is better and when.
BTW, there is an upper limit on number of links (32 by default)

[alexeyzimarev]

Makes total sense. I am travelling now but I’d love to submit a PR for the first case. I already have the code. Also need to check if the benchmark tests cover context propagation. If not, I can add that too so it would be easier to compare.

[NatMarchand]

If I may, according to your tweet your code is :

this.entries = carrier is NoopDistributedContextCarrier || entries == null
     ? Enumerable.Empty<DistributedContextEntry>()
      : entries.Where(x => x != default).Distinct();

which is not equivalent to what is done in the current implementation.
You'll need to pass a comparer to the distinct in order to compare on the Key property value and not on the object itself.

As long as we're commenting, instead of defining an empty list, Array.Empty() could be used. static fields should be readonly as they're not mutated. Finally, one can consider using IReadOnlyCollection instead of IEnumerable for parameter in the ctor, as the type of the Entries property or at least for the entries field. I'd feel unsafe consuming a IEnumerable property as I'm not sure of what I can do with it. For example, I'm not sure I can enumerate multiple times without side effect or get a different instance each time I get the property. However, that's a change of API contract.

[alexeyzimarev]

The DistributedContextEntry already implements IEqutable that only compares
the key. I had a test that produced identical results but it might be
wrong, I need to check it.

On Thu, 19 Dec 2019 at 13:02, Nathanael Marchand [e-mail]
wrote:

If I may, according to your tweet your code is :

this.entries = carrier is NoopDistributedContextCarrier || entries == null
? Enumerable.Empty()
: entries.Where(x => x != default).Distinct();

which is not equivalent to what is done in the current implementation.
You'll need to pass a comparer to the distinct in order to compare on the
Key property value and not on the object itself.

As long as we're commenting, instead of defining an empty list,
Array.Empty() could be used. static fields should be readonly as they're
not mutated. Finally, one can consider using IReadOnlyCollection instead of
IEnumerable for parameter in the ctor, as the type of the Entries property
or at least for the entries field. I'd feel unsafe consuming a IEnumerable
property as I'm not sure of what I can do with it. For example, I'm not
sure I can enumerate multiple times without side effect or get a different
instance each time I get the property. However, that's a change of API
contract.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/open-telemetry/opentelemetry-dotnet/issues/407?email_source=notifications&email_token=AAVQYVNVF6HVQTHV6HH2SO3QZNPDRA5CNFSM4J4KTL4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHJMN6I#issuecomment-567461625,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAVQYVKVNJUZL6T7NGWKGRTQZNPDRANCNFSM4J4KTL4A
.

[NatMarchand]

DistributedContextEntry does not implements anything. It override Equals but compares Key and Value. DistributedContext however does impment IEquatable<DistributedContext>
(as of commit e304133)

[alexeyzimarev]

Than I am wrong and my test definitely missed something. I think it should
be ok to pass a static comparer instance and then it will work.

I would very much appreciate switching from IEnumerable to
IReadOnlyCollection. The N+1/2 issue will go away entirely. I think the
client should be responsible to enumerate the collection upfront to avoid
any side effect like IO or anything of that kind that IEnumerable
implicitly brings. That also applies to the second class in my question.

Last thing, it my tweet. I admit it was too harsh. That was not my original
intent and I only realised it in retrospective. I am sorry. This project is
great and you’re all awesome.

On Thu, 19 Dec 2019 at 13:29, Nathanael Marchand [e-mail]
wrote:

DistributedContextEntry does not implements anything. It override Equals
but compares Key and Value. DistributedContext however does impment
IEquatable
(as of commit e304133
https://github.com/open-telemetry/opentelemetry-dotnet/commit/e304133b03e1796219f5faba1472565d2108bf11
)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/open-telemetry/opentelemetry-dotnet/issues/407?email_source=notifications&email_token=AAVQYVPJPFKNLDEKVYQITRLQZNSMDA5CNFSM4J4KTL4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHJOSXI#issuecomment-567470429,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAVQYVIBGBLV2B4EH67PAITQZNSMDANCNFSM4J4KTL4A
.

[NatMarchand]

FYI, I only saw your tweet and came here because I was curious and tried to understand.
I don't work at all on the project, it was just kind remarks and humble opinion. :)
(Although, I'll be glad to help if need be)

[cijothomas]

Most of span/.activity iteration over enumerator has been replaced with alloc-free approaches. Closing this issue. Please reopen if you see more scope for improvement. We are always looking for ways to improve performance!