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

Skeleton of a tracer for AWS X-Ray #8526

Merged
merged 19 commits into from
Oct 22, 2019
Merged

Skeleton of a tracer for AWS X-Ray #8526

merged 19 commits into from
Oct 22, 2019

Conversation

marcomagdy
Copy link
Contributor

Signed-off-by: Marco Magdy mmagdy@gmail.com

Description:
A skeleton tracer to incrementally add support for AWS X-Ray
Risk Level: Low
Testing: unit tests for functionality in util - the rest of files have no business logic to test yet
Docs Changes: N/A
Release Notes: N/A

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8526 was opened by marcomagdy.

see: more, trace.

jmarantz
jmarantz reviewed Oct 8, 2019
View reviewed changes
source/extensions/tracers/xray/util.cc Outdated Show resolved Hide resolved
test/extensions/tracers/xray/util_test.cc Outdated Show resolved Hide resolved
source/extensions/tracers/xray/util.h Outdated Show resolved Hide resolved
source/extensions/tracers/xray/xray_tracer_impl.cc Outdated Show resolved Hide resolved
jmarantz
jmarantz reviewed Oct 8, 2019
View reviewed changes
test/extensions/tracers/xray/util_test.cc Outdated Show resolved Hide resolved
source/extensions/tracers/xray/util.h Outdated Show resolved Hide resolved
source/extensions/tracers/xray/util.cc Show resolved Hide resolved
jmarantz
jmarantz previously approved these changes Oct 9, 2019
View reviewed changes
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

lgtm modulo issues of consistency with envoy config practice (config with inlined data) vs AWS sdk practice (config with ref to filename).

I'll let @htuch make the call.

@marcomagdy
Skeleton of a tracer for AWS X-Ray
6b85164 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
Fix proto format
bbadcb4 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
Not sure why gcc did not catch all this
b17c374 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
Fix clang-tidy errors
fa410cc 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
review feedback
7be86ba 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
Add a TODO and remove blank line
7464153 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
more test cases for wildcard matching
276b079 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
Use DataSource instead of string filename
7ad797c 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
Copy link
Contributor Author

I've changed the file_name to be a DataSource

htuch
htuch suggested changes Oct 10, 2019
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

/wait

source/extensions/tracers/xray/sampling_strategy.h Outdated Show resolved Hide resolved
source/extensions/tracers/xray/tracer.h Outdated Show resolved Hide resolved
source/extensions/tracers/xray/util.cc Outdated Show resolved Hide resolved
source/extensions/tracers/xray/util.cc Outdated Show resolved Hide resolved
source/extensions/tracers/xray/util.h Show resolved Hide resolved
test/extensions/tracers/xray/util_test.cc Outdated Show resolved Hide resolved
api/envoy/config/trace/v2/trace.proto Show resolved Hide resolved
@marcomagdy
Add tests for config loading
ab92985 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
Address feedback
553166a 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
clang tidy stuff
49323b5 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
crossing my fingers
f21aa1c 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
Docs - comment out references to xray
d4242f5 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy marcomagdy requested a review from htuch October 11, 2019 18:45
htuch
htuch suggested changes Oct 13, 2019
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

/wait

source/extensions/tracers/xray/util.cc Outdated Show resolved Hide resolved
source/extensions/tracers/xray/config.cc Outdated Show resolved Hide resolved
@marcomagdy
Fuzz test the wildcard matcher
e87a0e0 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
Use DataSource API
4abd180 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
Updated error message
c081d68 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy marcomagdy requested a review from htuch October 15, 2019 02:19
@marcomagdy
Added comments to the wildcard matching function
2a5f7e2 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
htuch
htuch suggested changes Oct 16, 2019
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Great, LGTM modulo a couple of test asks. The differential fuzzing is optional, but an idea that would make sense long term, in particular if we want to use this elsewhere in Envoy.
/wait

source/extensions/tracers/xray/sampling_strategy.h
}

private:
std::mt19937 rng_;
Copy link
Member

Choose a reason for hiding this comment

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

FYI, this is totally fine for this PR, but for the real implementation, you should obtain randomness via a Runtime::RandomGenerator (I think from factory context), to allow proper dependency injection and deterministic tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a reasonable explanation for this. But we'll talk about it in the next PRs

test/extensions/tracers/xray/fuzz_test.cc Outdated Show resolved Hide resolved
test/extensions/tracers/xray/util_test.cc Show resolved Hide resolved
test/extensions/tracers/xray/fuzz_test.cc Outdated Show resolved Hide resolved
@marcomagdy
more tests
b16f0fb 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
Added TODO
5b9cb89 
Signed-off-by: Marco Magdy <mmagdy@gmail.com>
@marcomagdy
Copy link
Contributor Author

macos build seems to be stuck.
/retest

@repokitteh-read-only
Copy link

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #8526 (comment) was created by @marcomagdy.

see: more, trace.

@htuch
Copy link
Member

htuch commented Oct 18, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

htuch
htuch approved these changes Oct 18, 2019
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks! I'd be keen to see the discussed improvements around glob parsing land, but given this is an opt-in extensions and a skeleton, let's merge and continue to iterate.

@marcomagdy
Copy link
Contributor Author

Is there anything I have to do to get this PR merged?
Also, should I squash the commits? or is the person merging this PR expected to do the squashing?

@jmarantz
Copy link
Contributor

Don't squash (or force-push); that will happen on merge. Probably we need to re-execute the MacOS tests which have most likely flaked; I'll do that now.

@jmarantz
Copy link
Contributor

/azp run envoy-macos

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@htuch htuch merged commit f68368f into envoyproxy:master Oct 22, 2019
@lavignes lavignes mentioned this pull request Oct 23, 2019
Open
derekargueta pushed a commit to derekargueta/envoy that referenced this pull request Oct 24, 2019
@marcomagdy @derekargueta
Skeleton of a tracer for AWS X-Ray (envoyproxy#8526)
38c7350 
A skeleton tracer to incrementally add support for AWS X-Ray

Risk Level: Low
Testing: unit tests for functionality in util - the rest of files have no business logic to test yet

Signed-off-by: Marco Magdy <mmagdy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmarantz jmarantz jmarantz left review comments

@lavignes lavignes lavignes left review comments

@htuch htuch htuch approved these changes

Assignees

@jmarantz jmarantz

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marcomagdy @htuch @jmarantz @lavignes