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

Introduce annotations on Agent types #990

Merged
merged 9 commits into from
Oct 26, 2020

Conversation

russcam
Copy link
Contributor

@russcam russcam commented Oct 22, 2020

This PR introduces annotations onto the agent types to describe the constraints/properties of the APM server specification:

  • Replace use of [JsonConverter(typeof(TrimmedStringJsonConverter))] with a [MaxLength] attribute, and assign the JsonConverter in ElasticApmContractResolver.
  • Introduce RequiredAttribute and mark required properties as such.
  • Rename TrimmedStringJsonConverter to TruncateJsonConverter

Discussed with @gregkalapos about using Data Annotations for this purpose, as it contains some of the attributes that are needed and provides types to validate instances against their attributes, something that may be useful for folks that use the agent API for manual instrumentation.

@russcam russcam requested a review from gregkalapos October 22, 2020 07:24
@russcam
Copy link
Contributor Author

russcam commented Oct 22, 2020

We did also talk about replacing JsonPropertyAttribute with DataMemberAttribute as JSON.Net supports the latter, and it is serializer agnostic, with the intention that it would make it easier to move/support to System.Text.Json in future. Currently, System.Text.Json does not support DataMemberAttribute (dotnet/runtime#29975), so have held off on doing so.

@apmmachine
Copy link
Contributor

apmmachine commented Oct 22, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [russcam commented: jenkins run the tests please]

  • Start Time: 2020-10-26T02:16:21.570+0000

  • Duration: 60 min 59 sec

Test stats 🧪

Test Results
Failed 0
Passed 14172
Skipped 0
Total 14172

/// Specifies that a data field value is required.
/// </summary>
[AttributeUsage(AttributeTargets.Property)]
public sealed class RequiredAttribute : Attribute { }
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of interest - do we already have a test on the other PR that makes sure the usage of this attribute is in-sync with the APM Server schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not because it's a chicken/egg scenario in that we only know what is required in the schema but not on the types. The schema required fields are validated in #984 in the mock APM server when receiving integration test data.

Copy link
Contributor

@gregkalapos gregkalapos left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@russcam
Copy link
Contributor Author

russcam commented Oct 26, 2020

Failing test

Elastic.Apm.Tests.ApiTests.ConvenientApiTransactionTests.ConvenientApiTransactionTests.Elastic.Apm.Tests.ApiTests.ConvenientApiTransactionTests.AsyncTask

Error Message
Expected value to be greater or equal to 1.0, but found 0.976.

@russcam russcam force-pushed the feature/annotations branch from 01c23c5 to 64d0cbe Compare October 26, 2020 00:49
@codecov-io
Copy link

codecov-io commented Oct 26, 2020

Codecov Report

Merging #990 into master will increase coverage by 0.16%.
The diff coverage is 97.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #990      +/-   ##
==========================================
+ Coverage   79.86%   80.03%   +0.16%     
==========================================
  Files         136      136              
  Lines        6307     6325      +18     
==========================================
+ Hits         5037     5062      +25     
+ Misses       1270     1263       -7     
Impacted Files Coverage Δ
src/Elastic.Apm/Api/CapturedException.cs 44.44% <ø> (ø)
src/Elastic.Apm/Api/Container.cs 100.00% <ø> (ø)
src/Elastic.Apm/Api/Database.cs 100.00% <ø> (ø)
src/Elastic.Apm/Api/Destination.cs 100.00% <ø> (ø)
src/Elastic.Apm/Api/Http.cs 83.92% <ø> (ø)
...c/Elastic.Apm/Api/Kubernetes/KubernetesMetadata.cs 60.00% <ø> (ø)
src/Elastic.Apm/Api/Kubernetes/Node.cs 50.00% <ø> (ø)
src/Elastic.Apm/Api/Kubernetes/Pod.cs 66.66% <ø> (ø)
src/Elastic.Apm/Api/Node.cs 100.00% <ø> (ø)
src/Elastic.Apm/Api/Request.cs 85.00% <ø> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21f481a...64d0cbe. Read the comment docs.

@russcam
Copy link
Contributor Author

russcam commented Oct 26, 2020

jenkins run the tests please

@russcam russcam merged commit a4b5c33 into elastic:master Oct 26, 2020
@russcam russcam added v1.7.0 enhancement New feature or request labels Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v1.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants