Skip to content

Add msgpack Packer based off msgpack-python 0.6.2#3028

Merged
mergify[bot] merged 1 commit intoDataDog:masterfrom
Kyle-Verhoog:old-packer
Nov 24, 2021
Merged

Add msgpack Packer based off msgpack-python 0.6.2#3028
mergify[bot] merged 1 commit intoDataDog:masterfrom
Kyle-Verhoog:old-packer

Conversation

@Kyle-Verhoog
Copy link
Copy Markdown
Member

@Kyle-Verhoog Kyle-Verhoog commented Nov 23, 2021

Commit Message

{{title}}

It's handy to have a general msgpack encoder that can be used for
encoding arbitrary payloads of primitive Python types (see #2915).

The encoder added here is based off the one added in #1491.

It might be useful to use as a fallback for encoding traces as well if
an issue with the custom msgpack encoder is suspected.

The relevant tests from the msgpack-python implementation are included
as well to ensure that the implementation is correct.

Checklist

  • Added to the correct milestone.
  • Tests provided or description of manual testing performed is included in the code or PR.
  • Library documentation is updated.
  • Corp site documentation is updated (link to the PR).

@Kyle-Verhoog Kyle-Verhoog added the changelog/no-changelog A changelog entry is not required for this PR. label Nov 23, 2021
@Kyle-Verhoog Kyle-Verhoog force-pushed the old-packer branch 5 times, most recently from 9747137 to dd467b6 Compare November 23, 2021 23:36
It's handy to have a general msgpack encoder that can be used for
encoding arbitrary payloads of primitive Python types (see DataDog#2915).

It might be useful to use as a fallback for encoding traces as well if
an issue with the custom msgpack encoder is suspected.
@Kyle-Verhoog Kyle-Verhoog marked this pull request as ready for review November 24, 2021 03:37
@Kyle-Verhoog Kyle-Verhoog requested a review from a team as a code owner November 24, 2021 03:37
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #3028 (23847b2) into master (dbcc631) will increase coverage by 0.10%.
The diff coverage is 97.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3028      +/-   ##
==========================================
+ Coverage   84.36%   84.46%   +0.10%     
==========================================
  Files         624      633       +9     
  Lines       45397    45745     +348     
==========================================
+ Hits        38298    38638     +340     
- Misses       7099     7107       +8     
Impacted Files Coverage Δ
tests/vendor/msgpack/test_read_size.py 89.83% <89.83%> (ø)
tests/vendor/msgpack/test_unpack.py 94.44% <94.44%> (ø)
tests/vendor/msgpack/test_buffer.py 100.00% <100.00%> (ø)
tests/vendor/msgpack/test_case.py 100.00% <100.00%> (ø)
tests/vendor/msgpack/test_except.py 100.00% <100.00%> (ø)
tests/vendor/msgpack/test_limits.py 100.00% <100.00%> (ø)
tests/vendor/msgpack/test_pack.py 100.00% <100.00%> (ø)
tests/vendor/msgpack/test_seq.py 100.00% <100.00%> (ø)
tests/vendor/msgpack/test_subtype.py 100.00% <100.00%> (ø)

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 dbcc631...23847b2. Read the comment docs.

Copy link
Copy Markdown
Contributor

@P403n1x87 P403n1x87 left a comment

Choose a reason for hiding this comment

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

🙌 cool!

return 0


cdef class Packer(object):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This source file is already quite big. Would it make sense to put this into its own source file?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah I considered it but opted not to because, iirc, adding another Cython module significantly increases compilation time 😞. There's also quite a bit of dependence that Packer has that is shared with the other encoders (the underlying C struct and all the methods)

@mergify mergify Bot merged commit 86e1f3e into DataDog:master Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/no-changelog A changelog entry is not required for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants