RuntimeTools / SwiftMetrics

Swift Application Metrics instruments the Swift runtime for performance monitoring, providing the monitoring data programatically via an API or visually with an Eclipse Client.
https://developer.ibm.com/swift/monitoring-diagnostics/application-metrics-for-swift/
Other
155 stars 50 forks source link

Remove JSON manipulation from SwiftMetricsDash Fixes #185 #188

Closed jonathan-spruce closed 6 years ago

jonathan-spruce commented 6 years ago

Removes the JSON used in the SwiftMetricsDash, which was causing a memory leak in Swift 4.1.2.

seabaylea commented 6 years ago

Rather than using using string templates, it would be much better (and safer!) to define Codable structs and then use the JSONEncoder from Foundation to create the payload, something like (not compiled!)

// Typesafe structs for CPU Data and wrapper object
struct CpuData: Codable {
    let process: Float
    let systemMean: Float
    let processMean: Float
    let time: Date
    let system: Float
}

struct Cpu: Codable {
    let topic = "cpu"
    let payload: Cpu
}

let cpu = Cpu(CpuData(.....))
let encoder = JSONEncoder()
let data = try encoder.encode(cpu)
sjanuary commented 6 years ago

@jonathan-spruce I actually did that work some time ago but it got taken out due to a clash with a more urgent piece of work. This might help you though: https://github.com/RuntimeTools/SwiftMetrics/pull/174/files

jonathan-spruce commented 6 years ago

There still seems to be a memory leak in the JSONEncoder, at least in the way I used it, so I have reverted back to the String implementation.

codecov-io commented 6 years ago

Codecov Report

Merging #188 into master will decrease coverage by 0.21%. The diff coverage is 31.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
- Coverage   22.95%   22.73%   -0.22%     
==========================================
  Files           9        9              
  Lines        2100     2111      +11     
==========================================
- Hits          482      480       -2     
- Misses       1618     1631      +13
Flag Coverage Δ
#SwiftBAMDC 0% <ø> (ø) :arrow_up:
#SwiftMetrics 22.73% <31.35%> (-0.22%) :arrow_down:
#SwiftMetricsBluemix 20.15% <97.36%> (-0.11%) :arrow_down:
#SwiftMetricsDash 17.95% <31.35%> (-0.23%) :arrow_down:
#SwiftMetricsKitura 19.56% <31.35%> (-0.23%) :arrow_down:
#SwiftMetricsPrometheus 18.06% <31.35%> (-0.21%) :arrow_down:
#SwiftMetricsREST 22.73% <31.35%> (-0.22%) :arrow_down:
Impacted Files Coverage Δ
Sources/SwiftMetricsDash/SwiftMetricsDash.swift 0% <0%> (ø) :arrow_up:
Sources/SwiftMetrics/SwiftMonitor.swift 86.47% <97.36%> (+0.16%) :arrow_up:
Sources/SwiftMetrics/SwiftMetrics.swift 62.9% <0%> (-0.54%) :arrow_down:

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 6e05488...35441c3. Read the comment docs.

sjanuary commented 6 years ago

I would not normally merge a fix with a failing Travis build, but I can see that the master build started failing in the same way about 10 days ago and it's important that we get this memory leak fix in. I will open another issue to investigate the test failures.