Skip to content

Conversation

@aaron-steinfeld
Copy link
Contributor

Based off of:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/semantic_conventions/deployment_environment.md

A few questions when looking in this code (@rish691 I think you have the most context):

  • Why are we using enums rather that static final constants? It seems like we're either using .getValue() everywhere or just saving them into constants anyway like
    private static final String OTEL_NET_PEER_IP = OTelSpanSemanticConventions.NET_PEER_IP.getValue();
  • Given that we're not actually using this in enrichment today, are there any other expected changes here? It seems like the utils around this are just wrappers for the enrichers.

@aaron-steinfeld aaron-steinfeld requested a review from a team January 28, 2021 19:58
@codecov
Copy link

codecov bot commented Jan 28, 2021

Codecov Report

Merging #124 (b09d612) into main (642dc0a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #124   +/-   ##
=========================================
  Coverage     69.79%   69.79%           
  Complexity      873      873           
=========================================
  Files            87       87           
  Lines          3615     3615           
  Branches        375      375           
=========================================
  Hits           2523     2523           
  Misses          946      946           
  Partials        146      146           
Flag Coverage Δ Complexity Δ
unit 69.79% <ø> (ø) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 642dc0a...b09d612. Read the comment docs.

@github-actions

This comment has been minimized.

@aaron-steinfeld aaron-steinfeld merged commit 6f16442 into main Jan 28, 2021
@aaron-steinfeld aaron-steinfeld deleted the add-deployment-env-constant branch January 28, 2021 21:07
@github-actions
Copy link

Unit Test Results

  50 files  ±0    50 suites  ±0   48s ⏱️ -2s
254 tests ±0  254 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 6f16442. ± Comparison against base commit 642dc0a.

@findingrish
Copy link
Contributor

findingrish commented Jan 29, 2021

Why are we using enums rather that static final constants?

No strong reasons, only that there were already a bunch of constants defined in enum classes https://github.com/hypertrace/hypertrace-ingester/tree/main/span-normalizer/span-normalizer-constants/src/main/java/org/hypertrace/core/span/normalizer/constants, so I just followed the pattern. Also the fact these constants are used in the platform directly, I felt following the existing pattern of defining constants would be better.

Given that we're not actually using this in enrichment today, are there any other expected changes here?

What kind of changes are you referring to?

It seems like the utils around this are just wrappers for the enrichers.

Well the intention was to expose the constants only through these wrapper methods for the consumers. But, yeah right now it is only used in enrichers. #71

@aaron-steinfeld
Copy link
Contributor Author

No strong reasons, only that there were already a bunch of constants defined in enum classes

Beyond being easier to use, constants indicate that they should not be enumerated, and more importantly - they're inlined by the compiler. This means they could be used as compileOnly dependencies, rather than runtime. Not sure how much work that would be to switch, and likely not worth adding more confusion to the pile right now.

What kind of changes are you referring to?

I was getting at the utils methods, but looks like you mentioned them below. I'm more interested in using them in view generators right now - we apply enrichment on some attributes, but others, we want to use as is - there's no logic to apply in enrichment and we just want it to go into the view. So in that case, is the view generator intended to use the util? the constant? something else?

@findingrish
Copy link
Contributor

findingrish commented Jan 29, 2021

So in that case, is the view generator intended to use the util? the constant? something else?

As I said earlier, I wanted the utils module to be used for these, but its artifact isn't being published.
You can directly use the constants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants