Updates for OTEP 66 #38
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ferd
approved these changes
Jan 15, 2020
| @@ -30,42 +30,46 @@ | |||
| %% tracer access functions | |||
| -export([span_module/1]). | |||
|
|
|||
| -include_lib("opentelemetry_api/include/opentelemetry.hrl"). | |||
There was a problem hiding this comment.
Just a thing I noticed:
since the record definitions are done in the opentelemetry_api OTP app, and that we're going to see that one included into all kinds of dependencies far more often than the current app, we should expect far more frequent conflicts in versions if the internal structure is to ever change if anyone else is importing the .hrl file.
I don't know if it'd be interesting to find a way to generate a warning when someone who is not opentelemetry is including the file and sneaking a peek at the records' structure somehow. I.e. maybe defining a macro in erl_opts for this lib and the API, and if that macro isn't defined, we cause some error or issue through preprocessor macros and bad declarations (write an unused function like this_include_file_not_intended_for_non_opentelemetry_apps() -> ok. conditionally)
There was a problem hiding this comment.
Argh, I was about to say, "no, it shouldn't be needed", but it is here SPAN_KIND_* macros are defined.
We may just need to split this header.
I don't believe anyone but a SDK should have to include this header. And as I type that I realize, also exporters. Grr.
But regular instrumented apps should not use the records directly, ever.
There was a problem hiding this comment.
Oh, but you want to warn against including it since it shouldn't be included?
I misread that as you worrying about how people had to include it but versions of it could change.
That would be interesting if we could warn not to use internal structures. I'd also wonder if we could move the records to the SDK to be safe. I think in the API they are currently just used for patching in function headers to know if an argument is of that record type.
There was a problem hiding this comment.
That at least reduces the scope, yeah. You shouldn't have 50 apps depending on various exporters, and as long as the record definition is macro'd and not literal, then the compiler will work it fine. The problem is any explicit matching on records at the call site can cause major compatibility issues.
There was a problem hiding this comment.
Yeah, whichever, the idea is to make sure nobody hardcodes a record's structure in a way that will cause incompatibilities in a top-level project including it all. External function calls are similar but would crash at runtime only, but we can't do much for these.
There was a problem hiding this comment.
Yea, it is probably fine because of the limited use it is required in. But never know what people will try to do in their apps.
hauleth
pushed a commit
to hauleth/opentelemetry-erlang
that referenced
this pull request
Sep 21, 2020
tsloughter
added a commit
that referenced
this pull request
Sep 23, 2020
* Initial commit * api initial commit ot_tracer: behavour that an otel tracer must implement ot_span: behaviour an otel span must implement otel: friendly user api on top of ot_tracer and ot_span * ot_span: add type specs on functions * add functions for creating records and align with latest otel protos * update readme to describe use of api application * updates for fred's PR review * separate context propagation otep * add with_value function to run function with scoped context * add http propagation for baggage, correlation and trace * remove callbacks from otel module that were accidentally copeid from ot_span * fix type specs after dialyzing opentelemetry * fix baggage and correlations to/from http propagation * add verification checks that tracer and context implement behaviours * add optional config value to context manager like the tracer * add some additional type specs * make propagator funs fully qualified important for being able to run upgrades. without fully qualified functions the funs will eventually become badfuns after the third module reload. * update span start_opts to include attributes, sampling hint and start_time (#6) * Initial Elixir API (#3) * start on the elixir API * add .gitignore, address feedback * update span start_opts to include attributes, sampling hint and start_time (#6) * use VERSION file for application version * read version from version file Co-authored-by: Tristan Sloughter <t@crashfast.com> * remove baggage. it was removed from otep-66 (#9) * remove resource from span record (#12) * add CODEOWNERS file (#15) * Registering named tracers (#16) * add tracer provider interface for named tracers * update readme with use about named tracer * fix register_tracer type spec * move span record to sdk * add additional details about what register_tracer does * new registeration, plus add set_span/2, end_span/2 and replace otel with macros in tracer.hrl (#17) * support registering an application's tracer * add set_span/2, end_span/2 and replace otel with macros in tracer.hrl * update readme to use register_application_tracer/1 * Set Erlang Approvers as CODEOWNERS (#19) * Elixir API based on the Erlang macro api and application registered tracers (#18) * add set_span/2, end_span/2 and replace otel with macros in tracer.hrl * Elixir API macros to match the Erlang macros * include erlang docs in elixir docs (#20) * include erlang docs in elixir docs * make erlang_docs function in mix.exs more readable * make edocs before mix docs and add hex metadata * make use of is_recording in ot_span and update events field to latest spec (#21) * use is_recording to skip possibly expensive span ops * update Event type to otel beta spec * missed a couple of is_recording and events changes (#22) * remove old is_recording_events callback * fix add_events callback type spec to new events record type * fixing up mix.lock and use of event (#23) * update elixir opentelemetry api to replace time events with events * update mix lock file * expand the docs in open_telemtry.ex, the readme and add CONTRIBUTING.md (#24) * add elixir docs to OpenTelemetry module * add register_application_tracer function to OpenTelemetry module and elixir to readme * add readme section about including the SDK * add contributing doc * add docs to span.ex and tracer.ex * chore: cleanup Elixir code - Macros use `:bind_quoted` where possible - Remove needless aliases in tests * Add create_span and new end_span/2 (#27) * Share configuration between Rebar3 and Mix (#11) * ft: share configuration between Rebar3 and Mix This should reduce amount of places where the configuration mismatch can happen. Current solution uses Rebar3 configuration as a source of truth. TBD: - support for Rebar3 profile sharing * fix(docs): build pages for the Erlang files * chore: update mix.lock * a bit of cleanup (#29) * remove unused context with_value function * fix is_recording macro in tracer header * opentelemetry metrics API (#7) * opentelemetry metrics API This patch includes a behaviour for implementing the Meter SDK and API modules for calling the meter for creating and recording measurements. * Update src/ot_meter.erl Co-Authored-By: Arkadiusz Gil <arkgil@users.noreply.github.com> * cleanup of metrics api based on comments on PR * add Meter Provider * remove gauge * add meter macros and registration * add elixir meter api module * add register_application_* functions to OpenTelemetry module * add observer api * add elixir modules for counter, measure and observer api * add meter to bound instrument return to simplify api * updates to observer api * remove label set function * more simplification and some docs * fix meter lookup * add counter and measure specific macros * fix get_meter typespec and add miissing callbacks to noop meter * fix provider return values to booleans for registering traer/meter Co-authored-by: Arkadiusz Gil <arkgil@users.noreply.github.com> * take default argument in ot_ctx:ctx and some simple bug fixes (#38) * Add callbacks for getting the Resource from a tracer provider (#40) * handle codechange in ot_tracer_provider * add callback to get resource from tracer provider * fix spec for provider resource fetching as resource is defined in the SDK * bump VERSION file to 0.3.0 (#41) * add include dir to files published to hex (#42) * removing wts dependency (#43) * add hex metadata * remove use of wts. get time_offset at export instead of at creation * use system time in nano seconds for an event time * version bump to 0.3.1 (#44) * add otel exporter to list and add lists for integrations (#45) * update readme to use set_attribute not the old add_attribute (#50) * drop attributes and events that aren't a list (#51) * Repair typespec (#46) I'm pretty sure you didn't intend to document [`register_application_tracer/1`][rat1] as: > Current time in UNIX Epoch time, nanoseconds since 00:00:00 UTC on > 1 January 1970. ... but that's how it turned out, perhaps due to a slip-up in 54ce3ad. [rat1]: https://hexdocs.pm/opentelemetry_api/0.3.1/OpenTelemetry.html#register_application_tracer/1 Co-authored-by: Tristan Sloughter <t@crashfast.com> * fix bug where there is no current span ctx and update_name is called (#52) * replace create_span with start_inactive_span (#53) * Add accessors to deconstruct Span (#54) * Add accessors to deconstruct Span When trying to link traces across separate systems (either linked via HTTP calls or a message queue), having direct access to the TraceID and the SpanID make it easier to serialize this properly and recreate a Link on the receiving end. This PR adds support for deconstructing a Span Context to get access to these values. I've added a couple of tests but they aren't all passing, so could use a hand getting the test for establishing links to parent trace in shape. * Rework CT test * Use the right type on type specs * Removed test that should be part of an SDK instead * Fix issues/48 (#56) - OpenTelemetry removing `/"` and replacing this with `"` as it creates confusion. - Updated the example doc for the new API updates. Modified `OpenTelemetry.Span` Following changes were done. - Changes related to documentation example `add_event` - Updated `add_event/1` to `add_event/2` to accept `event_name` and `event_attributes`. * Update readme (#59) * version bump to 0.3.2 (#55) * Fixing the string formatting acrosst the project. (#62) Changes to be committed: modified: lib/open_telemetry/measure.ex modified: lib/open_telemetry/meter.ex modified: lib/open_telemetry/observer.ex modified: lib/open_telemetry/tracer.ex * Update span add_event docs variable name (#64) Seems like the variables on lines 9,10 (`ecto_attributes`, and `ecto_event`) are the same variable, but named differently? * Unspecified is not a valid span kind (#67) * move context implementation to api app (#66) * moving to github actions to use the same as the rest of OpenTelemetry org (#68) * fix sampler typespec to term() since sampler is in SDK app * move to github actions for CI * support passing just a span_ctx to link functions (#69) * support passing just a span_ctx to link functions * use covertool and codecov instead of coveralls * fix elixir otp version in main.yml * fix link/2 to take undefined atom and fix typespecs * correct docs for timestamp functions (#70) * Add http status helper (#78) * Add http status helper * Update module doc * Fix message fallback * Use macro * Update headers typing (#79) Co-authored-by: Tristan Sloughter <t@crashfast.com> * Add atom support (#80) * Remove CircleCI (#81) Github actions appears to be the CI system now * Metrics api 0.4.0 (#71) * remove uses of 'labelset' which was removed from the spec * initial work on updated metrics api 0.4.0 * rename instrument function to definition and add elixir api * prefix meter macros with ot_ * Update lib/open_telemetry/sum_observer.ex Co-authored-by: Tino Breddin <tolbrino@users.noreply.github.com> * Update lib/open_telemetry/updown_sum_observer.ex Co-authored-by: Tino Breddin <tolbrino@users.noreply.github.com> * add some docs to ot_instrument Co-authored-by: Tino Breddin <tolbrino@users.noreply.github.com> * ctx api update and add optional explicit ctx argument to start span functions (#83) * rename correlations to baggage * update context api * make context to an optional explicit argument to start_span * rename ot_ctx:ctx() type to ot_ctx:t() to be consistent * feat: move opentelemetry app under apps * fix: remove CircleCI configuration * fix(cover): upload coverage report for API * fix: run CT in CI as well as EUnit * chore(cover): properly mark each coverage report This should result with better reports as soon as coverage reports start to parse. Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com> Co-authored-by: Tristan Sloughter <t@crashfast.com> Co-authored-by: Zach Daniel <zachary.s.daniel@gmail.com> Co-authored-by: Greg Mefford <greg@gregmefford.com> Co-authored-by: Arkadiusz Gil <arkgil@users.noreply.github.com> Co-authored-by: Garth Kidd <garth@garthk.com> Co-authored-by: Leandro Ostera <leandro@ostera.io> Co-authored-by: Yatender Singh <yatender.nitk@gmail.com> Co-authored-by: Marc Delagrammatikas <delagrammatikas@gmail.com> Co-authored-by: Bryan Naegele <bryannaegele@users.noreply.github.com> Co-authored-by: Dave Lucia <davelucianyc@gmail.com> Co-authored-by: Tino Breddin <tolbrino@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This brings the implementation of OTEP 66 (open-telemetry/oteps#66) into line with the latest, and soon to be merged, design. Now propagated context is kept in a separate variable. This is especially good so there isn't the possibility to end local span and end up then updating/ending what is actually a remote span: https://github.com/open-telemetry/oteps/blob/3f5ef37f28a0a38a252c3c6b669f42c2dcb7ef79/text/0066-separate-context-propagation.md#extract