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

Add attribute linting and ecosystem support. #19

Closed

Conversation

garthk
Copy link

@garthk garthk commented Jul 1, 2019

I keep repeating the same Elixir code in my reporters, middleware, and unit tests. This commit extracts all that code, cleans it up, and makes it available for everyone else working with Opencensus in Elixir.

  • Alter Opencensus.Trace.with_child_span/2 attribute handling
  • Add Opencensus.Trace.put_span_attributes/1
  • Add Opencensus.Trace.async/1
  • Add Opencensus.Trace.async/3
  • Add Opencensus.Trace.await/1
  • Add Opencensus.Span struct derived from the record
  • Add Opencensus.Span.from/1
  • Add Opencensus.Span.load/1
  • Add Opencensus.SpanContext struct derived from the record
  • Add Opencensus.SpanContext.from/1
  • Add Opencensus.SpanContext.hex_trace_id/1
  • Add Opencensus.SpanContext.hex_span_id/1
  • Add Opencensus.TestSupport.SpanCaptureReporter
  • Add Opencensus.Attributes.process_attributes/1
  • Add Opencensus.Attributes.process_attributes/2
  • Add Opencensus.Attributes.default_attributes/1

The new attribute handling:

  • Drops span attributes with unsupported values

  • Flattens nested maps

  • Documents the value support limits imposed by the Opencensus wire protocol and :ocp.put_span_attribute/2

  • Preserves the :module, :file, :function, :line, and :default magic attributes

Misc:

  • mix credo
  • mix test.watch

I keep repeating the same Elixir code in my reporters, middleware, and unit
tests. This commit extracts all that code, cleans it up, and makes it
available for everyone else working with Opencensus in Elixir.

- **Alter `Opencensus.Trace.with_child_span/2` attribute handling**
- Add `Opencensus.Trace.put_span_attributes/1`
- Add `Opencensus.Trace.async/1`
- Add `Opencensus.Trace.async/3`
- Add `Opencensus.Trace.await/1`
- Add `Opencensus.Span` struct derived from the record
- Add `Opencensus.Span.from/1`
- Add `Opencensus.Span.load/1`
- Add `Opencensus.SpanContext` struct derived from the record
- Add `Opencensus.SpanContext.from/1`
- Add `Opencensus.SpanContext.hex_trace_id/1`
- Add `Opencensus.SpanContext.hex_span_id/1`
- Add `Opencensus.TestSupport.SpanCaptureReporter`
- Add `Opencensus.Attributes.process_attributes/1`
- Add `Opencensus.Attributes.process_attributes/2`
- Add `Opencensus.Attributes.default_attributes/1`

The new attribute handling:

- **Drops span attributes with unsupported values**

- **Flattens nested maps**

- Documents the value support limits imposed by the Opencensus wire protocol
  and `:ocp.put_span_attribute/2`

- Preserves the `:module`, `:file`, `:function`, `:line`, and `:default`
  magic attributes

Misc:

- `mix credo`
- `mix test.watch`
@garthk
Copy link
Author

garthk commented Jul 1, 2019

Called out the most controversial parts in bold. Strikes me it's irresponsible to let package developers set span attributes that'll result in their spans getting dumped. Even if we decide that's the job of the reporters, we'll still want to offer the code to get the heavy lifting right. The rest is just useful.

@garthk garthk requested a review from tsloughter July 1, 2019 23:00
@garthk garthk self-assigned this Jul 1, 2019
@codecov-io
Copy link

codecov-io commented Jul 2, 2019

Codecov Report

Merging #19 into master will increase coverage by 2.62%.
The diff coverage is 97.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
+ Coverage   95.45%   98.07%   +2.62%     
==========================================
  Files           2        6       +4     
  Lines          22       52      +30     
==========================================
+ Hits           21       51      +30     
  Misses          1        1
Impacted Files Coverage Δ
lib/opencensus/span.ex 100% <100%> (ø)
lib/opencensus/span_context.ex 100% <100%> (ø)
lib/opencensus/logger.ex 100% <100%> (ø) ⬆️
lib/opencensus/attributes.ex 100% <100%> (ø)
...b/opencensus/test_support/span_capture_reporter.ex 100% <100%> (ø)
lib/opencensus/trace.ex 88.88% <88.88%> (-5.56%) ⬇️

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 de8f1df...5965462. Read the comment docs.

@tsloughter
Copy link
Member

Can this also include @zachdaniel 's idea to have default attributes be a config option? Instead of all this confusion with a list of default attributes like [:line, :file, ...] just include them in the attributes if some opencensus_elixir configuration is set to true.

That or just always include them as we can say "don't use the Elixir macro" if you don't want the extra attributes.

@garthk
Copy link
Author

garthk commented Jul 2, 2019

I made all four compulsory in my internal macro. Checking app config and including them or excluding them by default is a decent enough middle ground.

@garthk garthk mentioned this pull request Jul 3, 2019
@garthk
Copy link
Author

garthk commented Jul 3, 2019

Split #20 out; will come back with another PR for attribute control.

@garthk garthk closed this Jul 3, 2019
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.

4 participants