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 io and formatter options to console exporter #412

Merged
merged 7 commits into from
Feb 13, 2020

Conversation

c24t
Copy link
Member

@c24t c24t commented Feb 12, 2020

Fixes #411.

@c24t c24t requested a review from a team February 12, 2020 19:32
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggesting this in order to be able to use the Format Specification Mini-Language.

@AntonKueltz
Copy link

Moving this to the top level discussion since it got added to a thread just as it was resolved -

I'm not sure format is the best fit for everything all use cases. As an example, one of the fields in the dictionary structure we need to output is the latency of the span in milliseconds which we calculate via round((end - start) / scale). In other words, we're doing data transforms rather than just formatting how existing objects are output.

I suppose it's not impossible to do this sort of thing in a format string but it quickly becomes fairly hard to read compared to a formatter function which takes a Span and returns a str. So passing a function seems more succinct and flexible than format strings. Are there any major advantages you see with the format string approach? Or perhaps any issues with the formatter function approach?

@ocelotl
Copy link
Contributor

ocelotl commented Feb 12, 2020

Moving this to the top level discussion since it got added to a thread just as it was resolved -

I'm not sure format is the best fit for everything all use cases. As an example, one of the fields in the dictionary structure we need to output is the latency of the span in milliseconds which we calculate via round((end - start) / scale). In other words, we're doing data transforms rather than just formatting how existing objects are output.

I suppose it's not impossible to do this sort of thing in a format string but it quickly becomes fairly hard to read compared to a formatter function which takes a Span and returns a str. So passing a function seems more succinct and flexible than format strings. Are there any major advantages you see with the format string approach? Or perhaps any issues with the formatter function approach?

Ok, I understand your concern. What I think is happening here is that we are asking the format to do more than just formatting, we are asking it to also calculate stuff (in other words, to produce a field). The ideal solution would be to have all the fields that we want to have formatted ready before asking for the formatter to format them. I'm thinking right now about how to keep these 2 things cleanly separated, will update soon.

@ocelotl
Copy link
Contributor

ocelotl commented Feb 12, 2020

Moving this to the top level discussion since it got added to a thread just as it was resolved -

I'm not sure format is the best fit for everything all use cases. As an example, one of the fields in the dictionary structure we need to output is the latency of the span in milliseconds which we calculate via round((end - start) / scale). In other words, we're doing data transforms rather than just formatting how existing objects are output.

I suppose it's not impossible to do this sort of thing in a format string but it quickly becomes fairly hard to read compared to a formatter function which takes a Span and returns a str. So passing a function seems more succinct and flexible than format strings. Are there any major advantages you see with the format string approach? Or perhaps any issues with the formatter function approach?

Also, in order not to block this PR with my format investigations, I have asked @c24t to disregard the mini language suggestions for the time being, I'll open a new PR later if I find them worthy.

@codecov-io
Copy link

codecov-io commented Feb 12, 2020

Codecov Report

Merging #412 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
+ Coverage   85.86%   86.08%   +0.21%     
==========================================
  Files          41       41              
  Lines        2080     2084       +4     
  Branches      242      242              
==========================================
+ Hits         1786     1794       +8     
+ Misses        223      219       -4     
  Partials       71       71
Impacted Files Coverage Δ
...sdk/src/opentelemetry/sdk/trace/export/__init__.py 89.05% <100%> (+2.58%) ⬆️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py 91.17% <0%> (+0.36%) ⬆️

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 2a54a93...e09df42. Read the comment docs.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, would be good to add an example in the docs, but that can be done separately.

@c24t c24t merged commit 19d573a into open-telemetry:master Feb 13, 2020
@c24t c24t deleted the console-exporter-options branch February 13, 2020 00:03
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Feb 17, 2020
mauriciovasquezbernal added a commit to kinvolk/opentelemetry-python that referenced this pull request Mar 2, 2020
19d573a ("Add io and formatter options to console exporter (open-telemetry#412)")
changed the way spans are printed by using write() instead of print().
In Python 3.x sys.stdout is line-buffered, so the spans were not being printed
to the console at the right timing.

This commit fixes that by adding an explicit flush() call at the end of the
export function , it also changes the default formatter to include a line break.

To be precise, only one of the changes was needed to solve the problem, but as
a matter of completness both are included, i.e, to handle the case where the
formatter chosen by the user doesn't append a line break.
toumorokoshi pushed a commit that referenced this pull request Mar 4, 2020
19d573a ("Add io and formatter options to console exporter (#412)")
changed the way spans are printed by using write() instead of print().
In Python 3.x sys.stdout is line-buffered, so the spans were not being printed
to the console at the right timing.

This commit fixes that by adding an explicit flush() call at the end of the
export function , it also changes the default formatter to include a line break.

To be precise, only one of the changes was needed to solve the problem, but as
a matter of completness both are included, i.e, to handle the case where the
formatter chosen by the user doesn't append a line break.
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 6, 2020
Rename TracerSource to TracerProvider (open-telemetry#441)

Following discussion in open-telemetry#434, align the name with the specification.

Co-authored-by: Chris Kleinknecht <[email protected]>

Fix new ext READMEs (open-telemetry#444)

Some of the new ext packages had ReStructuredText errors. PyPI rejected the uploads for these packages with:

HTTPError: 400 Client Error: The description failed to render for 'text/x-rst'. See https://pypi.org/help/#description-content-type for more information. for url: https://upload.pypi.org/legacy/

Adding attach/detach methods as per spec (open-telemetry#429)

This change updates the Context API with the following:

- removes the remove_value method
- removes the set_current method
- adds attach and detach methods

Fixes open-telemetry#420

Co-authored-by: Chris Kleinknecht <[email protected]>

Make Counter and MinMaxSumCount aggregators thread safe (open-telemetry#439)

OT Collector trace exporter (open-telemetry#405)

Based on the OpenCensus agent exporter.

Fixes open-telemetry#343

Co-authored-by: Chris Kleinknecht <[email protected]>

API: Renaming TraceOptions to TraceFlags (open-telemetry#450)

Renaming TraceOptions to TraceFlags, which is the term used to describe the
flags associated with the trace in the OpenTelemetry specification.

Closes open-telemetry#434

api: Implement "named" meters + Remove "Batcher" from Meter constructor (open-telemetry#431)

Implements open-telemetry#221.
Also fixes open-telemetry#394.

Stateful.py and stateless.py in metrics example folder are not changed to use the new loader in anticipation of open-telemetry#422 being merged first and removing them.

Lastly, moves InstrumentationInfo from trace.py in the sdk to utils.

Prepare to host on readthedocs.org (open-telemetry#452)

sdk: Implement observer instrument (open-telemetry#425)

Observer instruments are used to capture a current set of values at a point in
time [1].

This commit extends the Meter interface to allow to register an observer
instrument by pasing a callback that will be executed at collection time.
The logic inside collection is updated to consider these instruments and
a new ObserverAggregator is implemented.

[1] https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md#observer-instruments

sdk: fix ConsoleSpanExporter (open-telemetry#455)

19d573a ("Add io and formatter options to console exporter (open-telemetry#412)")
changed the way spans are printed by using write() instead of print().
In Python 3.x sys.stdout is line-buffered, so the spans were not being printed
to the console at the right timing.

This commit fixes that by adding an explicit flush() call at the end of the
export function , it also changes the default formatter to include a line break.

To be precise, only one of the changes was needed to solve the problem, but as
a matter of completness both are included, i.e, to handle the case where the
formatter chosen by the user doesn't append a line break.

jaeger: Usage README Update for opentelemetry-ext-jaeger (open-telemetry#459)

Usage docs for opentelemetry-ext-jaeger need to be updated after the change to `TracerSource` with v0.4. Looks like it was partially updated already.

Users following the usage docs will currently run into this error:
`AttributeError: 'Tracer' object has no attribute 'add_span_processor'`

api: Implementing Propagators API to use Context  (open-telemetry#446)

Implementing Propagators API to use Context.

Moving tracecontexthttptextformat to trace/propagation, as TraceContext is specific to trace rather that broader context propagation.

Using attach/detach for wsgi and flask extensions, enabling activation of the full context rather that activating of a sub component such as traces.

Adding a composite propagator.

Co-authored-by: Mauricio Vásquez <[email protected]>
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 17, 2020
Rename TracerSource to TracerProvider (open-telemetry#441)

Following discussion in open-telemetry#434, align the name with the specification.

Co-authored-by: Chris Kleinknecht <[email protected]>

Fix new ext READMEs (open-telemetry#444)

Some of the new ext packages had ReStructuredText errors. PyPI rejected the uploads for these packages with:

HTTPError: 400 Client Error: The description failed to render for 'text/x-rst'. See https://pypi.org/help/#description-content-type for more information. for url: https://upload.pypi.org/legacy/

Adding attach/detach methods as per spec (open-telemetry#429)

This change updates the Context API with the following:

- removes the remove_value method
- removes the set_current method
- adds attach and detach methods

Fixes open-telemetry#420

Co-authored-by: Chris Kleinknecht <[email protected]>

Make Counter and MinMaxSumCount aggregators thread safe (open-telemetry#439)

OT Collector trace exporter (open-telemetry#405)

Based on the OpenCensus agent exporter.

Fixes open-telemetry#343

Co-authored-by: Chris Kleinknecht <[email protected]>

API: Renaming TraceOptions to TraceFlags (open-telemetry#450)

Renaming TraceOptions to TraceFlags, which is the term used to describe the
flags associated with the trace in the OpenTelemetry specification.

Closes open-telemetry#434

api: Implement "named" meters + Remove "Batcher" from Meter constructor (open-telemetry#431)

Implements open-telemetry#221.
Also fixes open-telemetry#394.

Stateful.py and stateless.py in metrics example folder are not changed to use the new loader in anticipation of open-telemetry#422 being merged first and removing them.

Lastly, moves InstrumentationInfo from trace.py in the sdk to utils.

Prepare to host on readthedocs.org (open-telemetry#452)

sdk: Implement observer instrument (open-telemetry#425)

Observer instruments are used to capture a current set of values at a point in
time [1].

This commit extends the Meter interface to allow to register an observer
instrument by pasing a callback that will be executed at collection time.
The logic inside collection is updated to consider these instruments and
a new ObserverAggregator is implemented.

[1] https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md#observer-instruments

sdk: fix ConsoleSpanExporter (open-telemetry#455)

19d573a ("Add io and formatter options to console exporter (open-telemetry#412)")
changed the way spans are printed by using write() instead of print().
In Python 3.x sys.stdout is line-buffered, so the spans were not being printed
to the console at the right timing.

This commit fixes that by adding an explicit flush() call at the end of the
export function , it also changes the default formatter to include a line break.

To be precise, only one of the changes was needed to solve the problem, but as
a matter of completness both are included, i.e, to handle the case where the
formatter chosen by the user doesn't append a line break.

jaeger: Usage README Update for opentelemetry-ext-jaeger (open-telemetry#459)

Usage docs for opentelemetry-ext-jaeger need to be updated after the change to `TracerSource` with v0.4. Looks like it was partially updated already.

Users following the usage docs will currently run into this error:
`AttributeError: 'Tracer' object has no attribute 'add_span_processor'`

api: Implementing Propagators API to use Context  (open-telemetry#446)

Implementing Propagators API to use Context.

Moving tracecontexthttptextformat to trace/propagation, as TraceContext is specific to trace rather that broader context propagation.

Using attach/detach for wsgi and flask extensions, enabling activation of the full context rather that activating of a sub component such as traces.

Adding a composite propagator.

Co-authored-by: Mauricio Vásquez <[email protected]>
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 26, 2020
Rename TracerSource to TracerProvider (open-telemetry#441)

Following discussion in open-telemetry#434, align the name with the specification.

Co-authored-by: Chris Kleinknecht <[email protected]>

Fix new ext READMEs (open-telemetry#444)

Some of the new ext packages had ReStructuredText errors. PyPI rejected the uploads for these packages with:

HTTPError: 400 Client Error: The description failed to render for 'text/x-rst'. See https://pypi.org/help/#description-content-type for more information. for url: https://upload.pypi.org/legacy/

Adding attach/detach methods as per spec (open-telemetry#429)

This change updates the Context API with the following:

- removes the remove_value method
- removes the set_current method
- adds attach and detach methods

Fixes open-telemetry#420

Co-authored-by: Chris Kleinknecht <[email protected]>

Make Counter and MinMaxSumCount aggregators thread safe (open-telemetry#439)

OT Collector trace exporter (open-telemetry#405)

Based on the OpenCensus agent exporter.

Fixes open-telemetry#343

Co-authored-by: Chris Kleinknecht <[email protected]>

API: Renaming TraceOptions to TraceFlags (open-telemetry#450)

Renaming TraceOptions to TraceFlags, which is the term used to describe the
flags associated with the trace in the OpenTelemetry specification.

Closes open-telemetry#434

api: Implement "named" meters + Remove "Batcher" from Meter constructor (open-telemetry#431)

Implements open-telemetry#221.
Also fixes open-telemetry#394.

Stateful.py and stateless.py in metrics example folder are not changed to use the new loader in anticipation of open-telemetry#422 being merged first and removing them.

Lastly, moves InstrumentationInfo from trace.py in the sdk to utils.

Prepare to host on readthedocs.org (open-telemetry#452)

sdk: Implement observer instrument (open-telemetry#425)

Observer instruments are used to capture a current set of values at a point in
time [1].

This commit extends the Meter interface to allow to register an observer
instrument by pasing a callback that will be executed at collection time.
The logic inside collection is updated to consider these instruments and
a new ObserverAggregator is implemented.

[1] https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md#observer-instruments

sdk: fix ConsoleSpanExporter (open-telemetry#455)

19d573a ("Add io and formatter options to console exporter (open-telemetry#412)")
changed the way spans are printed by using write() instead of print().
In Python 3.x sys.stdout is line-buffered, so the spans were not being printed
to the console at the right timing.

This commit fixes that by adding an explicit flush() call at the end of the
export function , it also changes the default formatter to include a line break.

To be precise, only one of the changes was needed to solve the problem, but as
a matter of completness both are included, i.e, to handle the case where the
formatter chosen by the user doesn't append a line break.

jaeger: Usage README Update for opentelemetry-ext-jaeger (open-telemetry#459)

Usage docs for opentelemetry-ext-jaeger need to be updated after the change to `TracerSource` with v0.4. Looks like it was partially updated already.

Users following the usage docs will currently run into this error:
`AttributeError: 'Tracer' object has no attribute 'add_span_processor'`

api: Implementing Propagators API to use Context  (open-telemetry#446)

Implementing Propagators API to use Context.

Moving tracecontexthttptextformat to trace/propagation, as TraceContext is specific to trace rather that broader context propagation.

Using attach/detach for wsgi and flask extensions, enabling activation of the full context rather that activating of a sub component such as traces.

Adding a composite propagator.

Co-authored-by: Mauricio Vásquez <[email protected]>
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
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.

Custom output function in console exporter
5 participants