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

Public cleanup #78

Merged
merged 4 commits into from
Jan 28, 2019
Merged

Public cleanup #78

merged 4 commits into from
Jan 28, 2019

Conversation

gregkalapos
Copy link
Contributor

@gregkalapos gregkalapos commented Jan 25, 2019

The purpose of this change is to make sure that the agent initially only has a minimal public API surface. Therefore lots of public things became internal. The intention is to avoid braking changes later. #77

  • IDiagnosticListener and all its implementors became internal. IDiagnosticsSubscriber and its implementors remain public. This way from the user's perspective it's easier to know how to subscribe: just use the subscribers, and don't care about the listeners, that's handled internally.

  • Lots of classes from Elastic.Apm.Model.Payload became internal.

  • Introduced IError interface, mainly because the Error class leaked lots of internal information that is not relevant to users.

  • Removed a couple of properties from ITransaction and ISpan, which could potentially cause problems when users set data that is not accepted by the server. Those properties are available on Transaction and Span.

  • Due to the previous point I added internal Span StartSpanInternal to Transaction and internal Transaction StartTransactionInternal to Tracer. These can be used in agent dlls to access properties that are not on the public interface.

  • Plus added code signing to SampleAspNetCoreApp.csproj to avoid build warnings. (It is referenced from a test project, which is signed, so it also should be signed, which it is from now on).

This project is referenced in Elastic.Apm.AspNetCore.Tests (which is also signed). To avoid building with warnings the sample project will be also signed.
@codecov-io
Copy link

codecov-io commented Jan 25, 2019

Codecov Report

Merging #78 into master will increase coverage by 0.52%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage    81.6%   82.13%   +0.52%     
==========================================
  Files          35       35              
  Lines         957      957              
  Branches      126      125       -1     
==========================================
+ Hits          781      786       +5     
+ Misses        127      122       -5     
  Partials       49       49
Impacted Files Coverage Δ
src/Elastic.Apm/Model/Payload/Request.cs 100% <ø> (ø) ⬆️
src/Elastic.Apm/Model/Payload/Context.cs 100% <ø> (ø) ⬆️
src/Elastic.Apm/Model/Payload/Response.cs 100% <ø> (ø) ⬆️
...astic.Apm/Config/EnvironmentConfigurationReader.cs 100% <ø> (ø) ⬆️
src/Elastic.Apm/Model/Payload/Payload.cs 100% <ø> (ø) ⬆️
src/Elastic.Apm/Helpers/StacktraceHelper.cs 75% <ø> (ø) ⬆️
src/Elastic.Apm/Model/Payload/Stacktrace.cs 75% <ø> (ø) ⬆️
...DiagnosticListener/AspNetCoreDiagnosticListener.cs 75% <ø> (+5.76%) ⬆️
src/Elastic.Apm/Model/Payload/Span.cs 89.47% <ø> (ø) ⬆️
...Apm.AspNetCore/Config/MicrosoftExtensionsConfig.cs 74.28% <ø> (ø) ⬆️
... and 12 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 f3994e9...6cc5090. Read the comment docs.

@gregkalapos gregkalapos changed the title Public cleanup #77 Public cleanup Jan 25, 2019
Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

LGTM, just jotted down some random thoughts as possible follow up items.

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