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.

Sign up for GitHub

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

Jump to bottom

parametric: Add tests for OTEL #884

Merged
merged 56 commits into from Mar 28, 2023
Merged

parametric: Add tests for OTEL #884

merged 56 commits into from Mar 28, 2023

Conversation

dianashevchenko
Copy link
Contributor

@dianashevchenko dianashevchenko commented Feb 18, 2023 •
edited by ajgajg1134

Description

Adds a bunch of tests for OTEL APIs in the tracers. To start only the go library runs these tests. In reviewing, pay the most attention to ensuring the expectations are correct as this is the first set of tests here.

Workflow

  1. ⚠️⚠️ Create your PR as draft (we're receiving lot of PR, it saves us lot of time) ⚠️⚠️
  2. Follows the style guidelines of this project (See how to easily lint the code)
  3. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  4. Mark it as ready for review

Once your PR is reviewed, you can merge it ! ❤️

dianashevchenko and others added 30 commits January 27, 2023 19:12
@dianashevchenko
added structures for otel apps in parametric tests
61f1eaa
@dianashevchenko
updated protos
34436e8
@dianashevchenko
reformatted
4c9fdfc
@dianashevchenko
added more methods / method properties
4536a19
@dianashevchenko
Merge branch 'main' into shevchenko/otel-parametric
c9518dc
@dianashevchenko
added spancontext
65a1a30
@dianashevchenko
combined otel and regular tests into one entity
f783b48
@dianashevchenko
lint
b68f88b
@lievan
added test cases on top of new otel parametric structure
baf524f
@lievan
fixes to set status, flushing, isrecording
6605b5a
@lievan
set IsRoot default to false for starting span
9def536
@lievan
fix lint
fac4129
@dianashevchenko
cleaned up
9a751e2
@dianashevchenko
Merge pull request #857 from DataDog/shevchenko/otel-parametric
8aa1fec 
Created structure for Otel parametric tests
@dianashevchenko
Merge branch 'shared/otel-parametric' into evan.li/otel-parametric-v2
ad6b371
@lievan
refactored otel set attributes to allow for single-value attributes
13eae88
@lievan
check span resource name is the same as name
a59b251
@lievan
added starting span with attributes
d605e6d
@lievan
testing for tracer start options
43608cb
@lievan
implemented span timestamp end option, refactored test cases
2fdf41d
@lievan
refactored tests to be more granular
652cfb2
@lievan
remove redundancy in otel helper methods
609419a
@lievan
add clarifying comments and additional case to set status
96471dc
@dianashevchenko
Merged main into shared/otel-parametric
62d7263
@dianashevchenko
Added otel span methods specific tests
11eff8f
@dianashevchenko
Merged shared/otel-parametric into shevchenko/otel-span-tests
66bcd24
@dianashevchenko
fixed merge conflict
52905fb
@dianashevchenko
fixed faulty merge conflicts
cd1d830
@dianashevchenko
Merge branch 'shared/otel-parametric' into shevchenko/otel-span-tests
5abcbef
@dianashevchenko
fixed more span methods
e572273
katiehockman
katiehockman approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@katiehockman katiehockman left a comment

Choose a reason for hiding this comment

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

I didn't check every test super carefully yet, but overall it looks great. Thanks!

parametric/_library_client.py Outdated Show resolved Hide resolved
parametric/apps/golang/main.go Outdated Show resolved Hide resolved
parametric/apps/golang/otel_methods.go Outdated Show resolved Hide resolved
parametric/apps/golang/main.go Show resolved Hide resolved
parametric/apps/golang/otel_methods.go Outdated Show resolved Hide resolved
parametric/apps/golang/otel_methods.go Outdated Show resolved Hide resolved
parametric/test_otel_span_methods.py Show resolved Hide resolved
parametric/test_otel_span_methods.py Show resolved Hide resolved
parametric/test_otel_span_methods.py Outdated Show resolved Hide resolved
parametric/test_otel_tracer.py Outdated Show resolved Hide resolved
@dianashevchenko dianashevchenko removed the request for review from lievan March 22, 2023 12:03
pawelchcki
pawelchcki approved these changes Mar 23, 2023
View reviewed changes
Copy link
Collaborator

@pawelchcki pawelchcki left a comment •
edited

Choose a reason for hiding this comment

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

LGTM but - it would be good if either @Kyle-Verhoog or @cbeauchesne also would take a peek before this is merged

Qard
Qard reviewed Mar 24, 2023
View reviewed changes
Copy link
Contributor

@Qard Qard left a comment

Choose a reason for hiding this comment

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

Overall LGTM. I'll try it out with Node.js soon. 🙂

parametric/_library_client.py Outdated Show resolved Hide resolved
@cbeauchesne
Copy link
Collaborator

cbeauchesne commented Mar 24, 2023 •
edited

I can approve it, but @Kyle-Verhoog will have a way better feedback on parametric test than mine

Kyle-Verhoog
Kyle-Verhoog reviewed Mar 27, 2023
View reviewed changes
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

just a few nits but otherwise looks good to me.

@pawelchcki we should really look into changing over to http sooner rather than later since we're accumulating some debt here.

@dianashevchenko if you're up for it, we should also be implementing otel for the http interface as we're looking to get rid of grpc in order to support all the languages.

parametric/_library_client.py Outdated Show resolved Hide resolved
parametric/test_otel_span_with_w3c.py Outdated Show resolved Hide resolved
parametric/test_otel_tracer.py Outdated Show resolved Hide resolved
@dianashevchenko dianashevchenko merged commit bb1d9f0 into main Mar 28, 2023
182 of 186 checks passed
@dianashevchenko dianashevchenko deleted the shevchenko/otel-span-tests branch March 28, 2023 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajgajg1134 ajgajg1134 ajgajg1134 left review comments

@Qard Qard Qard approved these changes

@pawelchcki pawelchcki pawelchcki approved these changes

@Kyle-Verhoog Kyle-Verhoog Kyle-Verhoog approved these changes

@cbeauchesne cbeauchesne cbeauchesne approved these changes

@katiehockman katiehockman katiehockman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@dianashevchenko @cbeauchesne @Qard @pawelchcki @ajgajg1134 @Kyle-Verhoog @katiehockman @lievan