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

feature: cucumber integration #1216

Merged
merged 37 commits into from
Oct 30, 2020
Merged

feature: cucumber integration #1216

merged 37 commits into from
Oct 30, 2020

Conversation

jirikuncar
Copy link
Contributor

@jirikuncar jirikuncar commented Oct 22, 2020

Adds integration for 🥒 Cucumber.

Appraisals Show resolved Hide resolved
@jirikuncar jirikuncar marked this pull request as ready for review October 23, 2020 15:02
@jirikuncar jirikuncar requested a review from a team October 23, 2020 15:02
Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

This approach generally seems ok but per my comments let's move using the include / prepend pattern here instead of class_eval.

@marcotc Can you take a look at the usage of Pin here? Is there a more appropriate pattern to use in this case?

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

I think the changes are looking good!

One thing I noticed is that some of the patterns used (like the Pin, or the Integration#patch implementation) are following patterns that we no longer want to follow.

Here's an example of a very recent integration: https://github.com/DataDog/dd-trace-rb/pull/1121/files

And the relevant part when it comes to creating the actual tracing span:

def call(deserialized_msg, delivery_info, metadata, handler)
trace_options = {
service: configuration[:service_name],
span_type: Datadog::Ext::AppTypes::WORKER
}
request_span = tracer.trace(Ext::SPAN_JOB, trace_options)
# Set analytics sample rate
if Datadog::Contrib::Analytics.enabled?(configuration[:analytics_enabled])
Datadog::Contrib::Analytics.set_sample_rate(request_span, configuration[:analytics_sample_rate])
end
# Measure service stats
Contrib::Analytics.set_measured(request_span)
request_span.resource = @app.to_proc.binding.eval('self.class').to_s
request_span.set_tag(Ext::TAG_JOB_ROUTING_KEY, delivery_info.routing_key)
request_span.set_tag(Ext::TAG_JOB_QUEUE, delivery_info.consumer.queue.name)
if configuration[:tag_body]
request_span.set_tag(Ext::TAG_JOB_BODY, deserialized_msg)
end
@app.call(deserialized_msg, delivery_info, metadata, handler)
rescue StandardError => e
request_span.set_error(e) unless request_span.nil?
raise e
ensure
request_span.finish if request_span
end

Overall, removing the Pin and simplifying the Integration class is what I see as left to bring this PR up to our latest patterns.

lib/ddtrace/contrib/cucumber/formatter.rb Outdated Show resolved Hide resolved
lib/ddtrace/contrib/cucumber/formatter.rb Show resolved Hide resolved
span_type: Datadog::Ext::AppTypes::TEST,
tags: pin.tags
}
@current_feature_span = pin.tracer.trace(Datadog::Ext::AppTypes::TEST, trace_options)
Copy link
Member

Choose a reason for hiding this comment

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

Can tests be nested in Cucumber? I'm wondering if it's possible that @current_feature_span is assigned a value more than once before being consumed in #on_test_case_finished.

lib/ddtrace/contrib/cucumber/formatter.rb Outdated Show resolved Hide resolved
lib/ddtrace/contrib/cucumber/patcher.rb Outdated Show resolved Hide resolved
lib/ddtrace/ext/app_types.rb Show resolved Hide resolved
@jirikuncar
Copy link
Contributor Author

@ericmustin thank you for the review.

Min version (were events not part of the api before 4.0?)

✅ there are some minor differences - in version 3 the events were sent for step hooks too.

Nested Steps

🗑️ not supported - not implemented

We need to add documentation

✅ done

@ericmustin
Copy link
Contributor

@jirikuncar sounds good, will try to have a final pass here before eod...i'll look into whatever those build issues are, seems unrelated to this PR

docs/GettingStarted.md Outdated Show resolved Hide resolved
marcotc
marcotc previously approved these changes Oct 29, 2020
ericmustin
ericmustin previously approved these changes Oct 30, 2020
Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

🚀 I left the most minor of nits, otherwise LGTM. nice work @jirikuncar !

docs/GettingStarted.md Outdated Show resolved Hide resolved
Co-authored-by: Eric Mustin <eric.mustin@datadoghq.com>
@jirikuncar jirikuncar dismissed stale reviews from ericmustin and marcotc via aecd51a October 30, 2020 13:09
@jirikuncar
Copy link
Contributor Author

@ericmustin good catch! Thanks

@marcotc marcotc added feature Involves a product feature integrations Involves tracing integrations labels Oct 30, 2020
@marcotc marcotc merged commit 817a082 into master Oct 30, 2020
@marcotc marcotc deleted the jirikuncar/cucumber branch October 30, 2020 19:17
@marcotc marcotc added this to the 0.43.0 milestone Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants