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

Collector 0.2.0 #1

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Collector 0.2.0 #1

wants to merge 3 commits into from

Conversation

pauljohanneskraft
Copy link
Collaborator

Collector 0.2.0

♻️ Current situation

There is no way to only collect metrics or traces. You always get both features at once without the choice of one.

Dependencies should only be part of a framework, if they are essential to them - which is not the case for NIO in MetricCollector.

There is no way of injecting/extracting context from a Foundation URLRequest.

💡 Proposed solution

Separate Collector into MetricCollector and TraceCollector with only relevant dependencies between the two. Collector is still a package that can be imported (essentially MetricCollector + TraceCollector).

Remove direct dependency on NIO for MetricCollector. We still have it in the list through SwiftPrometheus though.

Add Context Propagation for URLRequest right in the main repository.

Implications

Minor interface changes from NIO to async/await.

Testing

There are no tests so far.

Reviewer Nudging

The changes are actually quite minor, so you should be able to skim over all of them.

Copy link
Contributor

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

@pauljohanneskraft Thanks for the improvements to Collector!
I have changed the GitHub Action to use the latest Xcode beta (13.2) to get the tests running It supports building and testing Swift Concurrency features for older os versions. There are still some issues as listed in Apodini/Apodini#374 but that doesn't affect this PR as you don't use any Swift Concurrency features in the test targets. So changing xcode-version: latest-stable to xcode-version: latest in the regular builds should be fine for new.

@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #1 (41648c2) into develop (868eac8) will decrease coverage by 0.63%.
The diff coverage is 6.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop      #1      +/-   ##
==========================================
- Coverage     1.23%   0.60%   -0.64%     
==========================================
  Files           10      17       +7     
  Lines          162     666     +504     
==========================================
+ Hits             2       4       +2     
- Misses         160     662     +502     
Impacted Files Coverage Δ
Sources/JaegerCollector/Sender/JaegerSender.swift 0.00% <0.00%> (ø)
Sources/MetricCollector/Metric.swift 0.00% <ø> (ø)
...ources/PrometheusCollector/Metric+Prometheus.swift 0.00% <0.00%> (ø)
Sources/TraceCollector/Agent/BasicAgent.swift 0.00% <0.00%> (ø)
.../TraceCollector/ContextPropagation/Extractor.swift 0.00% <ø> (ø)
...s/TraceCollector/ContextPropagation/Injector.swift 0.00% <ø> (ø)
...lector/ContextPropagation/URLRequest+Context.swift 0.00% <0.00%> (ø)
Sources/TraceCollector/Span/Span.swift 0.00% <ø> (ø)
Sources/TraceCollector/Span/Tag.swift 0.00% <ø> (ø)
Sources/TraceCollector/Tracer/BasicTracer.swift 0.00% <ø> (ø)
... and 9 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 868eac8...41648c2. Read the comment docs.

@PSchmiedmayer
Copy link
Contributor

PSchmiedmayer commented Dec 6, 2021

@pauljohanneskraft You also have the permissions to merge the PR despite the SwiftLint warnings and CodeCov reports once you feel the PR is ready. All important builds seem to pass now 🚀

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.

2 participants