Messaging convention reviewed

[iNikem]

Closes #983
Closes #1031

[trask]

😅 I didn't get through everything, but have a couple of initial comments

[trask]

Are these only used as string constants? If so string constants in JMSTracer seems more natural

[iNikem]

Having string input parameter allows for passing any string. In this case only these 3 values are allowed by convention.

[trask]

Makes sense 👍

Btw, the lowercase enum names are throwing me a bit, can we change to SEND("send")? I think that's more conventional.

[iNikem]

Can I answer you "no"? :) The "convention" to have enum elements all in uppercase baffles me. Why should we wrap desired values (lowercase in this case as per spec) in identical uppercase strings? What benefit that brings?

[trask]

The "convention" to have enum elements all in uppercase baffles me

Curious if you disagree that it is conventional for enums to be uppercase in Java? Or if you disagree that we should follow this convention?

The point of following convention is that it helps people reading the code. When I see Operation.send in code, it looks like direct non-constant field access, and takes me more time to understand than it's worth.

Also we document in CONTRIBUTING.md that we follow Google Java Style Guide so would suggest we lean on that for style / convention questions where possible.

Happy to accept @anuraaga as tie-breaker vote either way though 👍😄

[iNikem]

I find strange the notion "constants should be UPPERCASE in Java". But in general that is indeed, not important and we should just follow coding style.

But it is more important, IMO, when such coding style forces clumsier design. And I think that SEND("send") is clumsy.

[anuraaga]

Generally LGTM

[iNikem]

Closes #983

[iNikem]

@trask I have filed open-telemetry/opentelemetry-specification#1085. It also contains my proposal, how parent-child relationship should be modelled for messaging spans.

[iNikem]

@trask Updated parents as well. I mostly agreed with you, as you can probably see from my spec issue :)

[trask]

👍

Last remaining comment is about the uppercase enums.

[iNikem]

👍

Last remaining comment is about the uppercase enums.

Yeah, I still don't like the idea of wrapping actual values just to have uppercase names.

[trask]

Yeah, I still don't like the idea of wrapping actual values just to have uppercase names.

It's not "just to have uppercase names". It's to make your code easier for other people to read.

When you go against a widely followed convention like this, you make your code harder for other people to read. E.g. when I was reviewing this PR and ran across the first reference, Operation.receive, I had to spend time understanding what that was, whereas Operation.RECEIVE would have immediately made sense (oh, a constant) and I could have just kept reading on.

[iNikem]

Yeah, I still don't like the idea of wrapping actual values just to have uppercase names.

It's not "just to have uppercase names". It's to make your code easier for other people to read.

When you go against a widely followed convention like this, you make your code harder for other people to read. E.g. when I was reviewing this PR and ran across the first reference, Operation.receive, I had to spend time understanding what that was, whereas Operation.RECEIVE would have immediately made sense (oh, a constant) and I could have just kept reading on.

(grumpy) ok-ok, make sense :( will do

[iNikem]

@trask No, I still don't like. You suggested to have SEND("send"). What will be name of that new field? I couldn't come up with a good option, whose usage would be natural.

And I don't like the idea of always doing Operation.SEND.name().toLowerCase(). Both overhead and it may break in some locales.

[iNikem]

Also, do you have any suggestions, how to merge your packages changes into this? :)

[anuraaga]

How about just using String constants instead of enum? A bit less typesafe but it's essentially what we do for semantic attributes. And actually, these constants should be in the SDK anyways :)

[anuraaga]

Sent open-telemetry/opentelemetry-java#1783

[iNikem]

Removed enum altogether, will replace with open-telemetry/opentelemetry-java#1783 after we switch to 0.10.0

[trask]

@iNikem I will fix the conflicts later today since I caused them 😁