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

Clean up POC #2839

Merged

Conversation

nr-ahemsath
Copy link
Member

The only real change is that instead of using a switch statement to convert request type names like "PutItemRequest" and "CreateTableRequest" to operation names like put_item and create_table, instead use a formula of "remove the trailing Request and then convert PascalCase to snake_case".

There was also a minor merge conflict in the new Memcache instrumentation which was created after the original POC was done.

@nr-ahemsath nr-ahemsath requested a review from a team as a code owner October 18, 2024 21:26
Copy link
Member

@chynesNR chynesNR left a comment

Choose a reason for hiding this comment

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

Looks good!

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Project coverage is 81.24%. Comparing base (33471e5) to head (b69bfea).

Files with missing lines Patch % Lines
...NewRelic.Agent.Extensions/Parsing/StringsHelper.cs 0.00% 23 Missing ⚠️
Additional details and impacted files
@@                         Coverage Diff                          @@
##           feature/dynamodb-instrumentation    #2839      +/-   ##
====================================================================
- Coverage                             81.31%   81.24%   -0.08%     
====================================================================
  Files                                   460      460              
  Lines                                 29224    29259      +35     
  Branches                               3231     3235       +4     
====================================================================
+ Hits                                  23764    23771       +7     
- Misses                                 4668     4694      +26     
- Partials                                792      794       +2     
Flag Coverage Δ
Agent 82.13% <0.00%> (-0.09%) ⬇️
Profiler 73.33% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...NewRelic.Agent.Extensions/Parsing/StringsHelper.cs 77.88% <0.00%> (-22.12%) ⬇️

... and 12 files with indirect coverage changes

@nr-ahemsath nr-ahemsath merged commit e12dba2 into feature/dynamodb-instrumentation Oct 21, 2024
90 of 93 checks passed
@nr-ahemsath nr-ahemsath deleted the featurework/cleanup-dynamodb-poc branch October 21, 2024 17:06
nr-ahemsath added a commit that referenced this pull request Oct 29, 2024
* Bring initial POC to feature branch (#2836)

* First pass at DynamoDB support

* db.system attribute was not being set correctly

---------

Co-authored-by: chynesNR <chynes@newrelic.com>

* Clean up POC (#2839)

* Remove datastore vendor name from ConnectionInfo in MemcachedHelpers

* Get operation name from request type by converting PascalCaseRequest to snake_case

* Make operation name cache thread safe (#2841)

* Make operation name cache thread safe

* Cleaner implementation based on PR feedback

* Update src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AwsSdk/DynamoDbRequestHandler.cs

Co-authored-by: Chris Ventura <45495992+nrcventura@users.noreply.github.com>

---------

Co-authored-by: Chris Ventura <45495992+nrcventura@users.noreply.github.com>

* DynamoDB integration tests (#2854)

* Rename things in preparation to add another AWS SDK test type

* Add project dependencies

* More renaming

* Initial plumbing created

* More cleanup

* Forklift exerciser methods from standalone test app

* Working tests

* Cleanup

* Fix port assignment issue seen in CI

* Only wait up to two minutes for table to become active

* List ports in use in container tests host

For temporary troubleshooting

* Update tests/Agent/IntegrationTests/ContainerApplications/AwsSdkTestApp/AwsSdkExercisers/AwsSdkDynamoDBExerciser.cs

Co-authored-by: Marty T <120425148+tippmar-nr@users.noreply.github.com>

* Fix port conflict issue plus PR feedback

---------

Co-authored-by: Marty T <120425148+tippmar-nr@users.noreply.github.com>

* Add unit tests for ToSnakeCase() (#2859)

Unit tests for ToSnakeCase()

---------

Co-authored-by: chynesNR <chynes@newrelic.com>
Co-authored-by: Chris Ventura <45495992+nrcventura@users.noreply.github.com>
Co-authored-by: Marty T <120425148+tippmar-nr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants