Skip to content

Conversation

fabianfett
Copy link
Member

  • Removed overly verbose issue and pr templates
  • Removed documentation from README
  • Readme now links to spi documentation page
  • Added docc documentation
  • Removed last usages of TimeHistogram

- Removed overly verbose issue and pr templates
- Removed documentation from README
- Readme now links to spi documentation page
- Added docc documentation
- Removed last usages of `TimeHistogram`
Copy link
Contributor

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple of small nits


## Installation

``Prometheus`` is available through Swift Package Manager. To include it in your project add the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
``Prometheus`` is available through Swift Package Manager. To include it in your project add the
``Prometheus`` is available through the Swift Package Manager. To include it in your project add the

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure "the" is needed here.


The same functionality is also available for ``ValueHistogram`` and aggregating `Recorder`s.

[Swift Metrics]: https://github.com/apple/swift-metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this link here at the bottom?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a markdown feature that allows you to have all the links in the end of your doc. this way the links don't polute the markdown reading experience inline.

Copy link
Collaborator

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Looks ok, thx

/// A closure to modify the name and labels used in the Swift Metrics API. This allows users
/// to overwrite the Metric names in third party packages.
public var labelAndDimensionSanitizer: @Sendable (_ label: String, _ dimensions: [(String, String)]) -> (String, [(String, String)])
public var nameAndLabelSanitizer: @Sendable (_ label: String, _ dimensions: [(String, String)]) -> (String, [(String, String)])
Copy link
Contributor

Choose a reason for hiding this comment

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

the var is called nameAndLabelSanitizer but the parameters are label, dimensions
this is maybe inconsistent

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a bit confusing re Prometheus and Swift Metrics naming.

Swift Metrics -> Prometheus
label -> name
dimensions -> labels

myGauge.decrement()
```

Lastly, you can use your ``PrometheusCollectorRegistry`` to generate a Prometheus export as in the
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what 'as in' means here

Copy link
Contributor

@hamzahrmalik hamzahrmalik left a comment

Choose a reason for hiding this comment

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

lgtm

@fabianfett fabianfett enabled auto-merge (squash) September 22, 2023 10:07
@fabianfett fabianfett merged commit b7ed98c into swift-server:main Sep 22, 2023
@fabianfett fabianfett added this to the 2.0 milestone Sep 25, 2023
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.

4 participants