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

Fix Elasticsearch integration #211

Merged
merged 9 commits into from
Nov 15, 2018
Merged

Fix Elasticsearch integration #211

merged 9 commits into from
Nov 15, 2018

Conversation

lucaspimentel
Copy link
Member

This PR fixes two issues with the Elasticsearch integration:

  • Fix return type for dynamically generated method (caught by Sigil validation)
    • blocking issue, crashes app
    • this issue was hidden before because we didn't do IL validation and the integration tests would check for the object's type and do the right thing if it was a Task/Task<T> or not
  • Non-blocking: Convert http method from enum to string
    • non-blocking, we simply didn't add the http method tag
    • this issue was hidden by an empty catch

@lucaspimentel lucaspimentel added type:bug area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) labels Nov 15, 2018
@lucaspimentel lucaspimentel added this to the 0.5.1-beta milestone Nov 15, 2018
@lucaspimentel lucaspimentel self-assigned this Nov 15, 2018
@lucaspimentel
don't ignore exception in test
3f26f8c 
don't ignore the `UnexpectedElasticsearchClientException` type since we dropped the CatSnapshot tests
@lucaspimentel
explain why CatSnapshots is skipped
e1c8aab
@lucaspimentel lucaspimentel merged commit 2eb0b5c into develop Nov 15, 2018
@lucaspimentel lucaspimentel deleted the lpimentel/elasticsearch branch November 15, 2018 16:56
lucaspimentel added a commit that referenced this pull request Nov 19, 2018
@lucaspimentel
Fix Elasticsearch integration (#211)
e556c6e 
* change return type of `CallElasticsearchAsync()` from `TResponse` to `Task<TResponse>` (caught by Sigil)

* convert http method to string

* add profiler environment variables to elasticsearch sample app

* clean up code

* add test for "Search", remove "CatSnapshots" because it fails deserializing the response (client bug or server incompatibility?)

* await the task earlier so we don't bother using reflection to get `Result` if the task is faulted

* collect any unexpected exceptions and throw an `AggregateException` at the end to fail the test

* don't ignore exception in test

don't ignore the `UnexpectedElasticsearchClientException` type since we dropped the CatSnapshot tests

* explain why CatSnapshots is skipped
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmermet bmermet bmermet approved these changes

Assignees

@lucaspimentel lucaspimentel

Labels
area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) type:bug
Projects
None yet
Milestone
0.5.1-beta
Development

Successfully merging this pull request may close these issues.
2 participants
@lucaspimentel @bmermet