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

Packages pushing to MyGet are invalid. #565

Merged
merged 1 commit into from
Apr 2, 2020
Merged

Packages pushing to MyGet are invalid. #565

merged 1 commit into from
Apr 2, 2020

Conversation

CodeBlanch
Copy link
Member

This one took me on a journey today!

I consumed OpenTelemetry.Collector.AspNet, OpenTelemetry.Collector.Dependencies, & OpenTelemetry.Exporter.Jaeger from MyGet into a .NET 4.8 WebApplication.

On startup I would crash loading OpenTelemetry.Exporter.Jaeger because it was trying to load .NET Standard 2.1. That's weird, it should be loading .NET Standard 2.0. Project asset file looked good though, referencing OpenTelemetry.Exporter.Jaeger through the /lib/netstandard2.0/ path.

A bunch of Googling later, I decided to look at the assemblies in the package using a decompiler. Turns out the MyGet OpenTelemetry.Exporter.Jaeger nupkg /lib/netstandard2.0/ folder actually contains a binary targeting netstandard2.1!

I tracked this down to an issue in our MyGet pipeline. From the last one:

Copying file from "D:\a\1\s\src\OpenTelemetry.Exporter.Jaeger\obj\Release\netstandard2.0\OpenTelemetry.Exporter.Jaeger.dll" to "D:\a\1\a\OpenTelemetry.Exporter.Jaeger.dll".
Copying file from "D:\a\1\s\src\OpenTelemetry.Exporter.Jaeger\obj\Release\netstandard2.1\OpenTelemetry.Exporter.Jaeger.dll" to "D:\a\1\a\OpenTelemetry.Exporter.Jaeger.dll".

Basically last target wins because the folder structure isn't capable of storing all the different outputs.

I took a stab at fixing it on this PR but I don't have a way to test the pipeline sorry!

@SergeyKanzhelev SergeyKanzhelev merged commit 3f43c5a into open-telemetry:master Apr 2, 2020
@CodeBlanch CodeBlanch deleted the myget-package-issue branch April 2, 2020 16:15
@CodeBlanch
Copy link
Member Author

@SergeyKanzhelev FYI New package looks good as far as including the correct binaries of the libs for each target! New issue: Jaeger package isn't including Thrift.dll or listing it as a dependency. I'll see if I can fix that.

@CodeBlanch CodeBlanch mentioned this pull request Apr 2, 2020
Merged
@MikeGoldsmith
Copy link
Member

Good investigation work @CodeBlanch 👍

MikeGoldsmith pushed a commit to MikeGoldsmith/opentelemetry-dotnet that referenced this pull request Apr 9, 2020
@CodeBlanch @MikeGoldsmith
Attempting to fix the myget build pipeline so the packages come out v…
260c52b 
…alid on the other end. (open-telemetry#565)

I don't have a way to start a build from fork. So let's test right in master...
MikeGoldsmith added a commit to MikeGoldsmith/opentelemetry-dotnet that referenced this pull request Apr 9, 2020
@MikeGoldsmith
Merge branch 'master' of github.com:open-telemetry/opentelemetry-dotn…
6c5a0bf 
…et into context-propagation

* 'master' of github.com:open-telemetry/opentelemetry-dotnet:
  Clone Activity tags to Span Attributes (open-telemetry#572)
  add LS access token header to http request (open-telemetry#587)
  ASP.NET dependency collector fixes and cleanup. (open-telemetry#570)
  Fixed up Jaeger project so that correct Thrift.dll is included in the nupkg & ApacheThrift is listed as a dependency. (open-telemetry#566)
  Fixed CurrentBatches dictionary being modified while a flush is running. (open-telemetry#571)
  Implemented short-live bound instruments. (open-telemetry#547)
  updating README.md samples (open-telemetry#556)
  Attempting to fix the myget build pipeline so the packages come out valid on the other end. (open-telemetry#565)
  Adding method to EvictingQueue, updating SpanSdk to update if key exists (open-telemetry#557)
  Fixed the solution not building from dotnet build CLI. (open-telemetry#554)
  fixes open-telemetry#559 (open-telemetry#563)
  Wrapping up enumerables for open-telemetry#407 and some cleanup (open-telemetry#558)
  Added support for .NET Framework HttpClient & HttpWebRequest dependency collection. (open-telemetry#553)
  remove UpdateThreadArguments class from LastValueAggregatorTest (open-telemetry#552)
  Fix build (open-telemetry#548)
  AspNet request collector (open-telemetry#543)

# Conflicts:
#	OpenTelemetry.proj
#	samples/Exporters/AspNet/Properties/AssemblyInfo.cs
#	samples/Exporters/AspNet/Web.Debug.config
#	samples/Exporters/AspNet/Web.Release.config
#	src/OpenTelemetry.Api/Context/CorrelationContextBuilder.cs
#	src/OpenTelemetry.Api/Context/CorrelationContextEntry.cs
#	src/OpenTelemetry.Api/Context/DistributedContext.cs
#	src/OpenTelemetry.Api/Metrics/CounterMetric.cs
#	src/OpenTelemetry.Api/Metrics/NoOpCounterMetric.cs
#	src/OpenTelemetry/Metrics/CounterMetricSdk.cs
#	src/OpenTelemetry/Metrics/MeterSdk.cs
#	src/OpenTelemetry/Metrics/RecordStatus.cs
#	src/OpenTelemetry/Trace/SpanSdk.cs
#	test/OpenTelemetry.Collector.AspNet.Tests/xunit.runner.json
#	test/OpenTelemetry.Tests/Impl/Context/DistributedContextsScopeTest.cs
#	test/OpenTelemetry.Tests/Impl/Context/Propagation/DistributedContextDeserializationTest.cs
#	test/OpenTelemetry.Tests/Impl/Context/Propagation/DistributedContextSerializationTest.cs
#	test/OpenTelemetry.Tests/Impl/DistributedContext/DistributedContextBuilderTest.cs
#	test/OpenTelemetry.Tests/Impl/DistributedContext/DistributedContextTest.cs
#	test/OpenTelemetry.Tests/Impl/DistributedContext/Propagation/DistributedContextRoundtripTest.cs
#	test/OpenTelemetry.Tests/Impl/Metrics/CounterCleanUpTests.cs
#	test/OpenTelemetry.Tests/Impl/Metrics/MetricsTest.cs
SergeyKanzhelev added a commit that referenced this pull request Apr 14, 2020
@MikeGoldsmith @CodeBlanch
@SergeyKanzhelev @alexeyzimarev @eddynaka @cijothomas @brunomasantos @bruno-garcia
Context propagation - OTEP-66 (#462)
bc684ff 
* consolidate all files under DistributeContext to Context in API and SDK

* rename DistributedContext classes to CorrelationContext et al

* update missed test renaming

* re-add DistributedContext and refactor carrier to use it

* add IEquatable to DistributedContext

* AspNet request collector (#543)

* Added a collector for incoming ASP.NET requests.

* Unit tests.

* Code review.

* Attempting to get windows build working.

* Attempting to fix build.

* Attempting to get the build working.

* Attempting to get the build working.

* Attempting to fix the build.

* Attempting to get Linux build working.

* Attempting to get linux build working.

* Attempting to get linux build working.

* Attempting to get build working.

* Attempting to get all tests running on windows.

* Attempting to get tests running on windows.

* Attempting to get the myget build working.

* Indention fix in myget yml.

* Code review.

* Code review.

* Code review.

* Code review.

* Code review.

* Fix build (#548)

* fix build

* rename queue to pool

* do not pack tests

* try to switch to VSBuild

* rename CorrelationContextScopeTest to DistributedContextScopeTest

* remove UpdateThreadArguments class from LastValueAggregatorTest (#552)

* Added support for .NET Framework HttpClient & HttpWebRequest dependency collection. (#553)

* Added support for .NET Framework HttpClient & HttpWebRequest dependency collection.

* Code review feedback.

* Updated HttpHandlerDiagnosticListener with latest changes from dotnet/runtime. Added try/finally for span.End calls. Fixed missing span.End in Sql exception event.

* Fixed broken SqlClientTests.

* A few tweaks to README for recent collector changes.

* Tested Azure SDK exception path and updated the TODO comment.

* Wrapping up enumerables for #407 and some cleanup (#558)

* Wrapping up enumerables for #407 and some cleanup

* Fixing whitespaces

* One more

Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>

* fixes #559 (#563)

* Fixed the solution not building from dotnet build CLI. (#554)

Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>

* Adding method to EvictingQueue, updating SpanSdk to update if key exists (#557)

* Adding method to EvictingQueue, updating SpanSdk to update if key exists

* Adding Replacing test to EvictingQueue; Updating SpanTest; Renaming from Update to Replace

* Adding wrapper to AddOrUpdateAttribute, adding validation to index in Replace method

* Adding NoReplacing Test when element doesn't exist

Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>

* Attempting to fix the myget build pipeline so the packages come out valid on the other end. (#565)

I don't have a way to start a build from fork. So let's test right in master...

* updating README.md samples (#556)

* updating README.md samples

* adding missing variables to example

* updating samples in readme

Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>

* Implemented short-live bound instruments. (#547)

* Initial draft for implementing record cleanup. Implemented for longCounters only. One basic test as well.

* small fixes

* Test for double counter

* Double test counter

* Use instrument level lock

* Added comment.

* Tests fix

* minor

* Added new intermediate status NoPendingUpdate for instrument record. This greatly reduced the need to take locks

* comment

* Fixed CurrentBatches dictionary being modified while a flush is running. (#571)

Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>

* Fixed up Jaeger project so that correct Thrift.dll is included in the nupkg & ApacheThrift is listed as a dependency. (#566)

Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>

* ASP.NET dependency collector fixes and cleanup. (#570)

* Updated with latest bug fixes. Refactored to fit into OT solution.

* Ported bug fixes and unit tests.

* Code review.

* Unit test improvement.

* Code review.

* Fixed crashing TestServer on close.

Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>

* add LS access token header to http request (#587)

* Clone Activity tags to Span Attributes (#572)

Resolves #430

* Initial work for validation

* Changed activity tags creation

Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>

* consolidate all files under DistributeContext to Context in API and SDK

* rename DistributedContext classes to CorrelationContext et al

* update missed test renaming

* re-add DistributedContext and refactor carrier to use it

* Fix build (#548)

* fix build

* rename queue to pool

* do not pack tests

* try to switch to VSBuild

* replace CorrelationContext with DistributedContext in BinarySerializer

* revert whitespace fixes to simplify review

* Update src/OpenTelemetry.Api/Context/CorrelationContext.cs

Co-Authored-By: Bruno Garcia <bruno@brunogarcia.com>

* Update src/OpenTelemetry.Api/Context/DistributedContextBuilder.cs

Co-Authored-By: Bruno Garcia <bruno@brunogarcia.com>

Co-authored-by: Mikel Blanchard <mblanchard@macrosssoftware.com>
Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>
Co-authored-by: Alexey Zimarev <alex@zimarev.com>
Co-authored-by: Eddy Nakamura <eddynaka@gmail.com>
Co-authored-by: Eddy Nakamura <ednakamu@microsoft.com>
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
Co-authored-by: Bruno Santos <brunomiguelas@gmail.com>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@bruno-garcia bruno-garcia Awaiting requested review from bruno-garcia

@discostu105 discostu105 Awaiting requested review from discostu105

@lmolkova lmolkova Awaiting requested review from lmolkova

@MikeGoldsmith MikeGoldsmith Awaiting requested review from MikeGoldsmith

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CodeBlanch @MikeGoldsmith @SergeyKanzhelev