Introduce SwiftMetrics Shim #155

Sherlouk · 2021-04-02T20:05:35Z

Hey folks 👋

Was going to raise an issue but thought this would work best as a contribution! (Hopefully the first of many 👀)

Apple have created their own metrics API which has been adopted by a number of other packages including, but not limited to, Vapor - a prominent Server Side Swift platform.

I thought an ideal feature would be to enable OpenTelemetry clients to bootstrap Swift Metrics with a shim package. This shim essentially redirects the data to the OpenTelemetry API functions.

let meter: Meter = // ... Your existing code to create a meter
let metrics = OpenTelemetrySwiftMetrics(meter: meter)
MetricsSystem.bootstrap(metrics)

If this is adopted, we may wish to add OpenTelemetry to the README of their package alongside SwiftPrometheus and StatsD. That would be a PR on their repo.

Tried to follow similar patterns to other products, but let me know if you need things changing!

Potentially this could be renamed/re-homed to an "Importers" directory to follow more closely with the "Exporters" systems. This would demonstrate a clear route forward for future bridges such as a swift-log product once we add support for logs.

nachoBonafonte · 2021-04-05T09:54:17Z

Hi @Sherlouk,
Thanks a lot for your contribution, really nice idea and clear implementation. Let's merge it in the repository and we will think where to put it in the future (I will open a topic for the next Swift SIG meeting ). doc

Opentelemetry metrics spec is still experimental, but think this PR adds a lot of value to the project, even if we must rework it in the future if/when the spec changes. You can open the PR to the swift-metrics repository when/if you want.

Sherlouk · 2021-04-05T10:55:38Z

Thanks @nachoBonafonte! I had already created myself a Slack reminder for Thursday to attend the SIG meeting (though with daylight savings I'm currently trying to figure out if it's at 5pm or 6pm in the UK 😂)

nachoBonafonte · 2021-04-05T10:59:10Z

I will join at 6pm in Spain (5pm in UK), I hope the rest will follow 😂😂

ktoso · 2021-04-06T00:34:22Z

Very nice to see this, thanks @Sherlouk! I think such shim is very useful and it's great to have it here.

If there's any questions about swift-metrics and swift-distributed-tracing please feel free to ping me @nachoBonafonte, I'm maintaining those from the Apple side.

ktoso · 2021-04-06T00:35:41Z

Sources/SwiftMetricsShim/MetricHandlers.swift

+
+class SwiftHistogramMetric: RecorderHandler, SwiftMetric {
+
+    var metricName: String


why is this a var and not a let?

I believe it is a var because it exists as part of a requirement from the SwiftMetric protocol - and it's not currently possible to use a let in a protocol definition, to my knowledge?

I may look into adding private(set) to reduce the scope of misuse

ktoso · 2021-04-06T00:37:20Z

Sources/SwiftMetricsShim/SwiftMetricsShim.swift

+
+    private func storeMetric(_ metric: SwiftMetric) {
+        metrics[.init(name: metric.metricName, type: metric.metricType)] = metric
+    }


This isn't thread-safe?

Metrics objects can be created by various threads, resulting in calls to makeCounter etc, which in turn invokes storeMetric. This needs to be synchronized somehow, e.g. by a lock wrapping access to metrics.

Correct, I'll make sure this gets fixed in another pull request - I'll tag you in it once it's ready 🙂
Thank you for taking a look!

Ok cool, yep feel free to ping me on those 👍

Sherlouk · 2021-04-06T08:29:21Z

Hi @ktoso,

Thanks for the comments - I wasn't aware of swift-distributed-tracing, I'll be sure to check that out some time soon!

I'll open a new pull request as soon as possible addressing the lack of thread safety.

ktoso · 2021-04-06T08:37:00Z

Thanks for the comments - I wasn't aware of swift-distributed-tracing, I'll be sure to check that out some time soon!

The idea there would be the same with the shims you added here. Apple being Apple we want full control over the API that we're building into some of our core things because of some special use-cases we have in mind. We're aware of (and fans of) OTel both in general as well as this Swift implementation, and we also have a very minimal OTel tracing backend community implementation by @slashmo.

It would be fantastic to have the same kind of Shim to offer apple/swift-distributed-tracing to emit through to the opentelemetry-swift SDK!

@slashmo's impl is still beneficial though as it is very minimal, which matters for some of our use cases; so it does not need to be an "either/or" but bot implementations can happily co-exist I hope :-)

Please have a look at the repos, as we're still before finalizing them.

I'm also working on the Swift concurrency itself, and have implemented "task local values" which arespecifically designed for tracing in mind, and should be quite a game changer for instrumenting swift systems, see the proposal here: swiftlang/swift-evolution#1245 This is already implemented and merged on current nightly snapshots of Swift. So the APIs that exist in swift distributed tracing will change because they will make use of Swift's task local values.

I can imagine you'll want to also make use of them in here directly, as well as through swift-distributed-tracing.

Please feel free to reach out with any questions!

slashmo · 2021-04-06T08:52:45Z

we also have a very minimal OTel tracing backend community implementation
@ktoso

Thanks for mentioning my implementation. For reference, here's the link: https://github.com/slashmo/opentelemetry-swift

With this implantation I try to strike a balance between conforming to the OTel spec and being a good Swift Distributed Tracing / Swift Server citizen (e.g. leveraging NIO, minimal use of Foundation, and low memory consumption).

I'm also happy to talk about and/or contribute to the official OTel client.

Sherlouk · 2021-04-06T11:29:38Z

I'm also happy to talk about and/or contribute to the official OTel client.

I'd certainly love to see this! I think bridging between Apple's libraries and the OpenTelemetry package offers the best experience for everybody.

In the same way we have exporters for numerous backend, I think having multiple importers for different packages is also massively helpful.

I'll try and take some time to better look into both the underlying package as well as your OTel client as soon as possible and see if we can't formulate a plan to merge what's appropriate 👍

I can also bring it up at the SIG meeting to see what everyone else thinks!

nachoBonafonte · 2021-04-06T11:37:25Z

If there's any questions about swift-metrics and swift-distributed-tracing please feel free to ping me @nachoBonafonte, I'm maintaining those from the Apple side.

Hi @ktoso, thanks a lot for your review and thanks also for your offer, be sure we will ping you when needed 😊. We are now currently trying to release a beta version with the Opentelemetry API correct and stabilized, but adding support for apple instrumented libraries or technologies is really high in our priority list.

I'm also working on the Swift concurrency itself, and have implemented "task local values" which are specifically designed for tracing in mind, and should be quite a game changer for instrumenting swift systems, see the proposal here: swiftlang/swift-evolution#1245 This is already implemented and merged on current nightly snapshots of Swift. So the APIs that exist in swift distributed tracing will change because they will make use of Swift's task local values. I can imagine you'll want to also make use of them in here directly, as well as through swift-distributed-tracing.

Yes, I already saw the proposal some time ago, and it looks really cool. My only concern is about OS version support, since we want to keep compatibility with some older iOS and macOS version, and sometimes when new technologies are announced are usually not supported in previous OS's.

I'm also happy to talk about and/or contribute to the official OTel client.

Hi @slashmo, It would be extremely great. Currently some of the implementations are a bit naive (my fault), and are probably not very optimal. I was more centered in having the functionality and usability than on the perfect implementation. Having some experts eyes on it would be excellent

Add SwiftMetrics to OpenTelemetry Shim Product

f95ec9d

Sherlouk requested review from nachoBonafonte and SergeyKanzhelev as code owners April 2, 2021 20:05

nachoBonafonte approved these changes Apr 5, 2021

View reviewed changes

nachoBonafonte merged commit 6c21961 into open-telemetry:main Apr 5, 2021

Sherlouk deleted the sherlouk/SwiftMetricsShim branch April 5, 2021 10:51

ktoso reviewed Apr 6, 2021

View reviewed changes

Sherlouk mentioned this pull request Apr 6, 2021

Add Locks to SwiftMetricsShim #159

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce SwiftMetrics Shim #155

Introduce SwiftMetrics Shim #155

Sherlouk commented Apr 2, 2021 •

edited

Loading

nachoBonafonte commented Apr 5, 2021 •

edited

Loading

Sherlouk commented Apr 5, 2021

nachoBonafonte commented Apr 5, 2021

ktoso commented Apr 6, 2021 •

edited

Loading

ktoso Apr 6, 2021

Sherlouk Apr 6, 2021

ktoso Apr 6, 2021

Sherlouk Apr 6, 2021

ktoso Apr 6, 2021

Sherlouk commented Apr 6, 2021

ktoso commented Apr 6, 2021 •

edited

Loading

slashmo commented Apr 6, 2021

Sherlouk commented Apr 6, 2021

nachoBonafonte commented Apr 6, 2021 •

edited

Loading


		class SwiftHistogramMetric: RecorderHandler, SwiftMetric {

		var metricName: String

Introduce SwiftMetrics Shim #155

Introduce SwiftMetrics Shim #155

Conversation

Sherlouk commented Apr 2, 2021 • edited Loading

nachoBonafonte commented Apr 5, 2021 • edited Loading

Sherlouk commented Apr 5, 2021

nachoBonafonte commented Apr 5, 2021

ktoso commented Apr 6, 2021 • edited Loading

ktoso Apr 6, 2021

Choose a reason for hiding this comment

Sherlouk Apr 6, 2021

Choose a reason for hiding this comment

ktoso Apr 6, 2021

Choose a reason for hiding this comment

Sherlouk Apr 6, 2021

Choose a reason for hiding this comment

ktoso Apr 6, 2021

Choose a reason for hiding this comment

Sherlouk commented Apr 6, 2021

ktoso commented Apr 6, 2021 • edited Loading

slashmo commented Apr 6, 2021

Sherlouk commented Apr 6, 2021

nachoBonafonte commented Apr 6, 2021 • edited Loading

Sherlouk commented Apr 2, 2021 •

edited

Loading

nachoBonafonte commented Apr 5, 2021 •

edited

Loading

ktoso commented Apr 6, 2021 •

edited

Loading

ktoso commented Apr 6, 2021 •

edited

Loading

nachoBonafonte commented Apr 6, 2021 •

edited

Loading