Skip to content

Bring release/2.0 branch up-to-date with master#1956

Merged
lucaspimentel merged 121 commits into
release/2.0from
zach/update-release-2.0
Oct 28, 2021
Merged

Bring release/2.0 branch up-to-date with master#1956
lucaspimentel merged 121 commits into
release/2.0from
zach/update-release-2.0

Conversation

@zacharycmontoya

Copy link
Copy Markdown
Contributor

master commit: 39baaf6

@DataDog/apm-dotnet

andrewlock and others added 30 commits September 16, 2021 11:53
Likely culprit for test failures in the execution benchmarks (only happens on .NET 5 interestingly)
Used the parsed version of the query string

APPSEC-1404

Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com>
…T 4.5 fix (#1755)

* Add support for Module Entrypoint rewriting and .NET 4.5 fix

* Add environment variables to control loader options

* Fix warnings and linux build

* Debug logs on flat layout

* testing a change in the algorithm for flat layout

* Moar debugging data

* Add missing LogDebugIsEnabled flags

* Change feature flags to follow the tracer pattern
* Update SDK version to 5.0.401

* Fix build_in_docker.sh script

* Update version of alpine

alpine 3.12 is no longer supported by .NET 5. Update to 3.14 (which will be lowest supported for .NET 6)

* Disable Verify's clipboard and diff tool integration

Causes failures in Alpine as the xsel tool isn't available

* Try downgrading alpine for now
* Rename provider integration names to ADO.NET to use the same behavior as CallSite.

* Removes IntegrationName from IAdoNetClientData
* Refactor InstrumentationDefinitions generator.

* Update tracer/build/_build/PrepareRelease/GenerateIntegrationDefinitions.cs

Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com>

Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com>
* Fix percentages in coverage report

* Include a link to the online Code Coverage report in the code coverage comparison comment
* Don't duplicate DB context configuration in MassTransit config

* Remove unused variable

* Try increasing command timeout to avoid flaky test
`shared: true` should be set when multiple process may write to the managed log file.
This will automatically delete all but the specified "baseline" branch (1.27.1 currently) and the most recent 2 branches (the previous, plus the newly created benchmark branch) when auto-tagging the version bump commit.

This should ensure we only have 3 active benchmark branches at any one time, reducing the burden on CI

(The bash manipulation and renaming logic has all been tested and run locally)
Allow users to specify their own rules file to change how AppSec will analyze request.

Co-authored-by: Lucas Pimentel-Ordyna <lucas.pimentel@datadoghq.com>
…fferent AppDomains. (#1794)

* Fix duplicating integrations due to multiple AppDomains.
Fix memory leak on NativeCallTargetDefinition disposal.

* Add comments.

* fix locks.
…erent thread (#1783)

## Issue
The existing ASP.NET instrumentation uses an HttpModule to handle the BeginRequest event at the beginning of a request. The callback creates a new scope, stores it in the execution context via `Tracer.ActiveScope`, and stores it inside the `HttpContext.Items` dictionary. Except for a few integrations like ASP.NET MVC, ASP.NET Web API 2, and WCF, all other integrations will only correlate their spans to the root span when the `Tracer.ActiveScope` property (which uses the execution context) is populated.

The issue is that ASP.NET may switch threads when handling requests. When it does this, it will correctly set the `HttpContext.Current` property on the new thread but the execution context of the new thread IS NOT identical to the previous execution context, causing `Tracer.ActiveScope` to be `null` or possibly incorrect.

## Fix
The fix is to add additional automatic instrumentation in the request pipeline to reset the `Tracer.ActiveScope` property. We've identified that the methods `System.Web.ThreadContext.AssociateWithCurrentThread` and `System.Web.ThreadContext.DisassociateFromCurrentThread` are responsible for managing HttpContext assignments, so these methods can be instrumented to extract the scope placed in the HttpContext and appropriately store/remove the `Tracer.ActiveScope`.

## Changes
- Add CallSite and CallTarget instrumentation on the following `System.Web` methods:
  - `System.Web.ThreadContext.AssociateWithCurrentThread`: Gets the scope from the `HttpContext.Items` dictionary and sets the `Tracer.ActiveScope` property
  - `System.Web.ThreadContext.DisassociateFromCurrentThread`: Unsets the `Tracer.ActiveScope` property
- Add a regression test that deterministically forces a thread switch and ensures that the two spans are included in the same trace
This is a continuation of PR #1734 to ensure that we test the same routes in both the OWIN Web API 2 sample and the ASP.NET Web API 2 sample.

Changes:
- Add message handler requests to the OwinWebApi2 test cases with updated snapshots
- Add expected span counts to the test cases to ensure the test waits for the proper amount of spans from the MockTraceAgent
* Azure Functions Instrumentation

* Share code between diagnostic observer and middleware instrumentation

* Use generic constraint

* Skip diagnostic observer in functions

* Rebase and new instrumentation definitions structure

* Remove unused class

* New API

* Missed sharing RequestTrackingFeature
Previously we were using the active span's service name in the logs.
In other ILogger injectors we only use the "application"  service name.
This updates the ILogger integration to do the same.
* Add steps to clean the packages directory and obj files

This is a horrible hack to try and get some space back in our integration and regression tests. But sometimes you gotta do whatcha gotta do.

* Remove unused methods

* Fix incorrect path in GetProfilerProjectBin()

* Assume that we always have the tracer home directory (which can be overriden set with the environment variable TracerHomeDirectory

I decided _not_ to just use the default DD_DOTNET_TRACER_HOME here, as thought it might cause extra confusiong for local testing, but I'm open to suggestions on this one

Note that this requires you run `build BuildTracerHomer` before running integration tests _even in Visual Studio_ etc. This is not ideal, but was the quickest solution for now

* Stop copying the profiler around in integration tests etc

Removed all these properties:
ExcludeManagedProfiler
ExcludeNativeProfiler
LoadManagedProfilerFromProfilerDirectory
ManagedProfilerOutputDirectory
ProfilerOutputDirectory

* Don't reference Datadog.Trace indirectly in the AutomapperTest project

* Revert "Add steps to clean the packages directory and obj files"

This reverts commit 7fc1c10.

* Fix default env vars in docker-compose

* Fix Arm64 reference

* Remove unused variables

* Fix alpine Waf copying wrong version into tracer home
* Add json exporter to benchmarks

* Upload benchmark results as an artifact for later processing

* Ignore the build_data folder (used in CI)

* Add default exporters

* Remove manual exporters

* Fix exporter
…der / reset streams (#1778)

* Fixed postjson for HttpStreamRequest

* Fix headers

* Request could be null if nothing has been posted

* No need for headers

* using stringbuildercache

* missing implementation

* Changing logging request in case of error

* remove request content implementation

* removing impl from benchmark

* - local function to static function to avoid capturing
- request turned to local object
- some omitted config await false

* Missing configure await false

Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com>

Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com>
* Fix tracer path

* Ensure all sdks are installed.
Changes crank spans in the following ways:
- Sets tag `env=ci`
- Add `Crank.` prefix to the "Test Suite". For example, `calltarget.linux_arm64.arm64` will become `Crank.calltarget.linux_arm64.arm64`
Apparently it takes 15 mins to copy them!
* Fix a bug where app sec failed in debug log mode because of a failure formatting a value

Also,

- Move some logs over to structured logging
- Remove native log for dlerror
- Use http for samples

Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com>
kevingosse and others added 23 commits October 18, 2021 16:13
* Clean the workspace before building on ARM64

* Also clean during test jobs
* Upgrade to Alpine 3.14
* Define GENERATED_OBJ_FILES as EXTERNAL_OBJECT
Adds a file containing the tracer version number to help make constructing URLs easier
We have to include the package version in _all_ the tests. When the MySqlConnector sample is built for the multi-api packages, it's _not_ built for the default package, which causes failures on master only
It should be a metric
- Use an updated Windows build image for the GitLab CI build, so the new Profiler native bits can build (image was generated from PR DataDog/datadog-agent-buildimages#174)
- Clone the profiler repo into the working directory and set the appropriate symbolic links to trick the profiler repo to believe the tracer repo is in a sibling directory
- Update the GitLab build command to build the profiler assets and the beta MSI
- Set MSBuild property `Platform=AnyCPU` for the managed profiler build
- Set MSBuild property `SpectreMitigation=false` for the native profiler build
  - This can be changed later, but this is how the profiler is currently being built and it ensures that the GitLab CI runs the exact same build as all other platforms. The current setting also prevents a build break in GitLab CI that can only be fixed with another Windows build image modification 
- Propagate the `MonitoringHomeDirectory` to the beta MSI build, so we don't rely on the relative-path fallback
…zed. (#1916)

`ExecuteDelayed` is about preventing side effects from running the loader very early in the AppDomain life cycle
by delaying it towards a later point in the AppDomain Life cycle.

Example for a crash caused by this kind of side effect:

WCF applications using `BasicHttpsBinding` (note the "s" in https) were crashing with the continuous profiler attached.
Error:
_System.Configuration.ConfigurationErrorsException: Configuration binding extension 'system.serviceModel/bindings/basicHttpsBinding'
could not be found.Verify that this binding extension is properly registered in system.serviceModel/extensions/bindingExtensions and
that it is spelled correctly._

This was because the respective parts WCF configuration subsystem used `WebSocket.IsApplicationTargeting45()` to tweak their behavior
on different framework versions. In turn, `WebSocket.IsApplicationTargeting45()` calls the static method
`BinaryCompatibility.TargetsAtLeast_Desktop_V4_5()`. That method uses the static variable `s_map`. That, in turn, is initialized
by the static cctor, i.e. first time `BinaryCompatibility` the class is used.

This Assembly Loader calls `Array.Sort` while initializing its logger.
That, it turn, also uses the `BinaryCompatibility` class internally, to choose a backward-compatible sorting algorithm.
As a result those flags are initialized and cached when the loader is invoked. However, at that time, the AppDomain may not
be completely initialized. To initialize, the `BinaryCompatibility` cctor invokes `AppDomain.GetTargetFrameworkName()`, which,
in turn, calls `Assembly.GetEntryAssembly()`.

That API returns `null` when invoked too early in the AppDomain lifecycle.
As a result, a bogus target framework moniker is obtained (and cashed), and - in turn - the binary compatibility flags
are initialized incorrectly (and also cached). As a result, everything that relies on the binary compatibility flags
(or the target Framework moniker) may work in an unpredictable matter. This also leads to the WCF crash.

To mitigate that, inside of <c>Execute()</c> we inspect whether `Assembly.GetEntryAssembly()` returns `null` before we
start executing. If it does, we off-load the execution to a helper thread and returns immediately.
The helper thread runs the <c>ExecuteDelayed(..)</c> method: It sleeps and periodically checks
`Assembly.GetEntryAssembly()` until it no longer returns returns `null`. Then the Loader proceeds with its normal logic.

As a result, the target framework moniker and the binary compatibility flags are initialized correctly.
</summary>
<remarks>
The above logic is further specialized, depending on the kind of the current AppDomain and where the app is hosted:

* On non-default AD:
  we do not wait.

* On default AD, app NOT hosted in IIS::
  `GetEntryAssembly` initially returns null, but once the AD is fully initialized, it returns the correct value.
  So, we apply the above strategy: wait on a separate thread until `GetEntryAssembly` is not null and then execute the loader.

  As mentioned, it is required because some APIs need `GetEntryAssembly` to populate bin compat flags in the Fx.
   * The user does not need to specify a parameter for this, since we wait _until `GetEntryAssembly` is not null_.
   * This behavior is on by default, but all delaying may be disabled using `DD_INTERNAL_LOADER_DELAY_ENABLED=false`.

* On default AD, app IS hosted in IIS:
  `GetEntryAssembly` always returns null. It will always stay null, and there is no point delaying anything in that case.
  Even if we did delay, we would not have an end-condition for the wait as `GetEntryAssembly` always remains null forever.
   * So by default we do not wait on IIS.
   * As a precaution we support an _optional_ wait that use user can opt into by setting DD_INTERNAL_LOADER_DELAY_IIS_MILLISEC to
     a potitive number of milliseconds. (Since on IIS there is no exit condition to that delay, the option cannot be Boolean.)
…g a new one (#1927)

* Use GitHub API to hide old code coverage/benchmark reports when adding a new one

The PR quickly fills up with clutter now. This should alleviate the issue, while still retaining the comments for prosperity in case people need them.

Unfortunately, that functionality is only exposed via the GraphQL API, so had to use that

* Update tracer/build/_build/Build.GitHub.cs

Co-authored-by: Kevin Gosse <krix33@gmail.com>

Co-authored-by: Kevin Gosse <krix33@gmail.com>

Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com>
* Don't trigger builds for PRs that only touch the tracer/dependabot files

* Add a github action to automatically regenerate the package versions and start a test when dependabot detects changes
* Enable running Logs Injection tests on Linux

We really need to be running these so that we are testing multiple package versions

* Pass package version in LogInjection integration tests

We need to pass the package version so that we can get the path to the correct version of the application, when building the multi-package samples. Otherwise we are checking the wrong log files!

* Try and workaround NLog on Linux issues

No matter what I try, no matter which directory char we use etc, I just can't get NLog to behave when reading the xml config. As a workaround, just force it to put the logs in the correct place on linux by changing it in code

* Use older "default" versions for logs injection on .NET Framework

Windows only currently runs a single package version for integration tests. To increase the number of covered versions for logs-injection, use on of the .NET Framework-only packages when running on full framework.

* Fix NLog for configuration for NLog 4.6+

In NLog4.6+, LibLog adds the attributes to the MDLC instead of the MDC, so we need to use a new config.
* Utf 8 without byte marker

* Changes to comments: test without BOM in appsec tests + read-only field
* Refactor ModuleMetadata creation from all loaded modules to only the required ones.

* Adds std::future to the ModuleLoadFinished EnqueueProcessModule call.

* Process module for ReJIT in the same thread as ModuleLoadFinished

* Protect for shutdown deadlock.

* Add support for retrieving memory dumps on integration tests hangs

* Avoid calling RequestReJIT in the same thread of ModuleLoadFinished.

* remove unused local variable

* Change the memdump algorithm and disable it by default.

* Ensure the promise is resolved on shutdown.
)

* Give some time for the aspnetcore process to exit
* Let AspNetBase dispose the agent (The agent must be disabled after the target process shuts down)
* Fix build_in_docker.sh

This was broken when we moved things into the tracer folder and wasn't fixed correctly

* Always build AspNetCoreRazorPages when building single sample

If you try and build a single sample on linux using (for example)

```
./tracer/build_in_docker.sh Clean BuildTracerHome BuildAndRunLinuxIntegrationTests -SampleName "LogsInjection.NLog"
```

then the build will fail. That's because the linux integration tests depend on `Samples.AspNetCoreRazorPages`, but when a single sample name is provided, it won't be built.

This change ensures we _always_ build `Samples.AspNetCoreRazorPages`, even when a specific sample is specified using `-SampleName <name>`
* Add tests that we _can't_ duck type invalidly defined types

We have lots of tests that we _can_ duck type certain things, but there are many cases where we should throw when attempting to Duck Cast. If we don't, we risk accidentally thinking we've duck cast correctly when we haven't.

Obviously an even better approach would be to have an analyzer that detects this too, but that's a _lot_ more work

* Add checks for method resolution that Duck attribute works with generic parameter names

* Add some error tests for reverse duck typing

* Add failing test for reverse proxy where virtual methods already have a method body

* Add test that reverse-proxy properties work correctly (they don't currently)

* Fix uninitialized fields and properties in reverse proxy

Call base constructor if the proxy type is inheriting from a class or abstract class.

* Change type returned by DuckType.Create() to an object

If the ProxyType is a struct, then even if the implementation is correct, this will throw for struct proxies when it attempts to cast to an IDuckType (as the struct doesn't implement IDuckType). The cast is largely unnecessary anyway, and can be cast explicitly by the caller if necessary (and safe)

* Fix cases in field and property duck typing that were being too lenient

They were being lenient to handle _reverse_ duck typing scenarios, but we'll split those out for clarity and increase the strictness here

Also add some warnings where we don't (can't) know if a conversion is valid, so it would result in a runtime error accessing the member

* Add additional guard clauses for method duck typing

- Catch when an argument changes from generic to non-generic in a generic method
- Catch when the original method returns void, but the new method returns non-void

* Skip tests for which we can't (or won't) detect failures

(Primarily around return types and conversions)

* Split reverse-duck-typing from forward-duck-typing

While these cases are quite similar, there are a bunch of requirements and edge cases that make interleaving the implementation problematic. Some of the additional tests added highlighted this problem.

To try and make changes to the duck-typing code safer, this separates both the implementation and the API of reverse duck typing from "forward" duck typing. This also fixes some of the implementation issues that were previously causing failures

* Make [DuckReverseMethod] a mirror of [Duck] attribute

- Extracted the common features to a DuckAttributeBase
- Allow [DuckReverseMethod] to be used on properties
- Update method resolution to use the new attributes
- Fix method resolution for forward duck-typing when using Duck attribute and have generic parameters
- Fix method resolution for reverse duck-typing
- [DuckIgnore] is no longer used by reverse duck typing, as you must decorate all your reverse methods/properties anyway

* Fix [DuckReverseMethod] usages

* Update SelectTargetMethod implementation

- Fix case where the duck type attribute parameters don't match method parameters count
- Fix duck chaining from reverse duck types - need to do _reverse_ duck chaining here, not normal forward duck chaining
- Fix reverse duck typing not finding
- Fix invalid test (caught with above changes)

* Handle additional incorrect usage case and add additional exception tests

* Ensure IntegrationOptions catches DuckType exceptions when using DuckCast(Type), as well as DuckCast<T>

* Refactor DuckType.Methods.cs to reduce duplication

* Update Public API tests

This added some public DuckTyping methods. They could be made internal, but their equivalents are already public, so I think it makes more sense to keep them all public for now

* Update ExceptionsTests

Introduce ReverseProxyMustImplementGenericMethodAsGenericException to make it easier to debug the issue
Remove unused exception
Add missing tests for ReverseProxyMissingMethodImplementationException
Add some extra tests for code coverage purposes

* Update incorrect comments

* Fix public API

* Comments from code review

* Update reverse proxy properties implementation

Reverse proxies for methods was previously broken.

As for methods, when we're doing reverse proxying, we need to do the opposite ducktype/duckcast compared to forward proxying

This also removes the DuckKind.Field for reverse proxies (as that doesn't make sense)
Also, updated the "duck cast/duck chain" delegates to return the actual type that you end up with, for use by the caller.

* Handle case where [DuckReverseMethod] attribute has different number of parameters than method

This always points to a configuration error for reverse duck typing, whereas for forward duck typing this isn't _necessarily_ an error (as the target may have optional parameters, for example)

Add an explicit check for the reverse duck type case to catch this.

* Fix incorrect arguments in test

* Use public types so we can test net452 in reverse proxy error tests

* Fix incorrectly ignored `WrongArgumentTypeInterfaceImplementations.DuckChainReturnMethod` test

* Fix Public API tests

* Ensure we catch _all_ exceptions when creating a duck type

Co-authored-by: Tony Redondo <tony.redondo@datadoghq.com>
Just merged ea5e046, and subsequently discovered a typo in the comments.
Sorry about that.
This fixes the typo.
…1939)

It's an in-between step between TestAllMinorPackageVersions, and only testing the default samples, and is now the default for the multi-api package tests

In many cases, the results are the same as using the default samples, but it should give more test coverage in PRs generally
* Try fix github workflow when dependabot creates PRs

Also allow manual dispatch so we can update whenever we like

* Auto tag the version bump commit on `release/*` branches too

* Apply suggestions from code review

Co-authored-by: Zach Montoya <zach.montoya@datadoghq.com>

* Ensure we can run the workflow when doing manual bump

Co-authored-by: Zach Montoya <zach.montoya@datadoghq.com>
@zacharycmontoya zacharycmontoya added the area:builds project files, build scripts, pipelines, versioning, releases, packages label Oct 28, 2021
@zacharycmontoya zacharycmontoya self-assigned this Oct 28, 2021
@zacharycmontoya zacharycmontoya requested a review from a team as a code owner October 28, 2021 15:11
@lucaspimentel lucaspimentel merged commit e19d5bd into release/2.0 Oct 28, 2021
@lucaspimentel lucaspimentel deleted the zach/update-release-2.0 branch October 28, 2021 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:builds project files, build scripts, pipelines, versioning, releases, packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.