opentelemetry-python icon indicating copy to clipboard operation
opentelemetry-python copied to clipboard

Improve to json support

Open sernst opened this issue 1 year ago • 7 comments

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Introduce to_dict to the objects included in the existing JSON serialization process for ReadableSpan, MetricsData, LogRecord, and Resource objects. This includes adding to_dict to objects that are included within the serialized data structures of these objects. In places where repr() serialization was used, it has been replaced by a JSON-compatible serialization instead. Inconsistencies between null and empty string values were preserved, but in cases where attributes are optional, an empty dictionary is provided as well to be more consistent with cases where attributes are not optional and an empty dictionary represents no attributes were specified on the containing object.

These changes also included:

  1. Dictionary typing was included for all the to_dict    methods for clarity in subsequent usage.
  2. DataT and DataPointT were did not include the    exponential histogram types in point.py, and so those    were added with new to_json and to_dict methods as    well for consistency. It appears that the exponential    types were added later and including them in the types    might have been overlooked. Please let me know if that    is a misunderstanding on my part.
  3. OrderedDict was removed in a number of places    associated with the existing to_json functionality    given its redundancy for Python 3.7+ compatibility.    I was assuming this was legacy code for previous    compatibility, but please let me know if that's not    the case as well.
  4. to_dict was added to objects like SpanContext,    Link, and Event that were previously being    serialized by static methods within the ReadableSpan    class and accessing private/protected members.    This simplified the serialization in the ReadableSpan    class and those methods were removed. However, once    again, let me know if there was a larger purpose to    those I could not find.

Finally, I used to_dict as the method names here to be consistent with other related usages. For example, dataclasses.asdict(). But, mostly because that was by far the most popular usage within the larger community:

328k files found on GitHub that define to_dict functions, which include some of the most popular Python libraries to date: https://github.com/search?q=%22def+to_dict%28%22+language%3APython&type=code&p=1&l=Python

versus

3.3k files found on GitHub that define to_dictionary functions: https://github.com/search?q=%22def+to_dictionary%28%22+language%3APython&type=code&l=Python

However, if there is a preference for this library to use to_dictionary instead let me know and I will adjust.

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

How Has This Been Tested?

  • [x] Passes tox testing as outlined in CONTRIBUTING.md

Does This PR Require a Contrib Repo Change?

  • [x] No.

Checklist:

  • [x] Followed the style guidelines of this project
  • [x] Changelogs have been updated
  • [x] Unit tests have been added
  • [ ] Documentation has been updated

sernst avatar Jul 02 '23 13:07 sernst

@sernst Are you still working on this?

lzchen avatar Aug 16 '23 22:08 lzchen

@lzchen Yep, will re-engage here soon.

sernst avatar Aug 18 '23 12:08 sernst

Hi all, thank you for your contributions up to this point -- I think this is a positive PR. Can we work on getting it across the line? I'm happy to help too.

IMO these to_dict implementations are helpful: e.g. https://github.com/open-telemetry/opentelemetry-python/pull/3780/files#r1524771566

pmcollins avatar Mar 18 '24 20:03 pmcollins

I'm happy to pick it back up if others think it is useful. It didn't seem like there was much excitement for it, and I wasn't really excited to back support for 3.7 given that version was EoL by the Python community in general already. But I believe that the extended lifetime support for this project beyond the Python community's accepted EoL lifetime is now passed as well, which would simplify the process. Some decisions need to be made on the exact implementation though, and I'm not sure what comments made here are opinion versus decision. If some of that can be cleared up, I'd be happy to give it a refresh assuming that the code hasn't drifted too much to make it a redo.

sernst avatar Mar 18 '24 20:03 sernst

I'm happy to pick it back up if others think it is useful. It didn't seem like there was much excitement for it, and I wasn't really excited to back support for 3.7 given that version was EoL by the Python community in general already. But I believe that the extended lifetime support for this project beyond the Python community's accepted EoL lifetime is now passed as well, which would simplify the process. Some decisions need to be made on the exact implementation though, and I'm not sure what comments made here are opinion versus decision. If some of that can be cleared up, I'd be happy to give it a refresh assuming that the code hasn't drifted too much to make it a redo.

Awesome -- I think this repo would benefit from these changes.

And yes, since 3.7 is no longer supported that conversation probably can be resolved now. (I have resolved my comments above as well.)

Looks like we need to agree upon whether we use to_dict vs __iter__? to_dict seems a bit more straightforward and complementary to the existing to_json methods but I'd like to hear your thoughts @ocelotl.

pmcollins avatar Mar 19 '24 22:03 pmcollins

I'm happy to pick it back up if others think it is useful. It didn't seem like there was much excitement for it, and I wasn't really excited to back support for 3.7 given that version was EoL by the Python community in general already. But I believe that the extended lifetime support for this project beyond the Python community's accepted EoL lifetime is now passed as well, which would simplify the process. Some decisions need to be made on the exact implementation though, and I'm not sure what comments made here are opinion versus decision. If some of that can be cleared up, I'd be happy to give it a refresh assuming that the code hasn't drifted too much to make it a redo.

Awesome -- I think this repo would benefit from these changes.

And yes, since 3.7 is no longer supported that conversation probably can be resolved now. (I have resolved my comments above as well.)

Looks like we need to agree upon whether we use to_dict vs __iter__? to_dict seems a bit more straightforward and complementary to the existing to_json methods but I'd like to hear your thoughts @ocelotl.

We should not add a method named to_dict. Every other object that can be converted to a dictionary gets converted to a dictionary by using dict(), this should not be the exception.

BTW, I was working on a PR that would add __repr__ to all classes: #3662. Had to put it in standby for the time being but I think there is a pattern in the way #3662 represents objects as strings that could be used here too.

ocelotl avatar Mar 20 '24 17:03 ocelotl

Sounds fine to me.

pmcollins avatar Mar 20 '24 20:03 pmcollins