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

V2 rain #5

Closed
wants to merge 5 commits into from
Closed

V2 rain #5

wants to merge 5 commits into from

Conversation

inanna-malick
Copy link
Owner

PR so rain can leave comments

Copy link

@sunshowers sunshowers left a comment

Choose a reason for hiding this comment

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

Still WIP, but here's the first bit.

@@ -58,7 +58,7 @@ jobs:
name: Install and configure gh-pages
command: |
npm install -g --silent [email protected]
git config user.email "pkinsky+ci-build@gmail.com"
git config user.email "inanna+ci-build@recursion.wtf"

Choose a reason for hiding this comment

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

<3

.gitignore Outdated
@@ -1,3 +1,5 @@
/target
**/*.rs.bk
Cargo.lock
honeycomb.key
tracing-honeycomb/proptest-regressions/*

Choose a reason for hiding this comment

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

newline at end of file (configure your editor maybe?)

Cargo.toml Outdated
"tracing-distributed",
"tracing-honeycomb",
]

Choose a reason for hiding this comment

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

newline at end of file

Choose a reason for hiding this comment

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

still needs a newline at EOF (maybe configure this in your editor?)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved

As a tracing layer, `TelemetryLayer` can be composed with other layers to provide stdout logging, filtering, etc.

This crate is primarily intended to be used by people implementing their own backends. A concrete implementation using honeycomb.io as a backend is available at (TODO: link to tracing-honeycomb).

Choose a reason for hiding this comment

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

add this link (to github)?


This crate is primarily intended to be used by people implementing their own backends. A concrete implementation using honeycomb.io as a backend is available at (TODO: link to tracing-honeycomb).

## Implementation note

Choose a reason for hiding this comment

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

Implementation notes

tracing-distributed/README.md Outdated Show resolved Hide resolved
tracing-distributed/src/lib.rs Outdated Show resolved Hide resolved
tracing-distributed/src/lib.rs Outdated Show resolved Hide resolved
Copy link

@sunshowers sunshowers left a comment

Choose a reason for hiding this comment

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

This looks fantastic <3

Comment on lines 81 to 83
# filters:
# branches:
# only: master

Choose a reason for hiding this comment

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

TODO: uncomment out these lines

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Cargo.toml Outdated
"tracing-distributed",
"tracing-honeycomb",
]

Choose a reason for hiding this comment

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

still needs a newline at EOF (maybe configure this in your editor?)

README.md Outdated

tracing subscriber for use with honeycomb.io distributed tracing. Supports generating random trace IDs or recording known trace IDs on the current span.
[![License](https://img.shields.io/badge/license-MIT-green.svg)](LICENSE)

Choose a reason for hiding this comment

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

Could you add a CI badge here?

tracing-distributed/Cargo.toml Outdated Show resolved Hide resolved
Add the following to your Cargo.toml to get started.

```toml
tracing-honeycomb = 0.1.0-alpha-1

Choose a reason for hiding this comment

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

needs quotes around the version number, and also "0.1.0"?

Choose a reason for hiding this comment

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

this may still need to be addressed

@@ -0,0 +1,26 @@
[package]
name = "tracing-honeycomb"
version = "0.1.0-alpha-1"

Choose a reason for hiding this comment

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

version can just be "0.1.0"


### Testing

Since `TraceCtx::current_trace_ctx` and `TraceCtx::record_on_current_span` can be expected to return `Ok` as long as some `TelemetryLayer` has been registered as part of the layer/subscriber stack and the current span is active, it's valid to `.expect` them to always succeed & to panic if they do not. However, you probably don't want to publish telemetry while running unit or integration tests.

Choose a reason for hiding this comment

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

could you rewrite this a little?

tracing-honeycomb/src/honeycomb.rs Show resolved Hide resolved
Copy link

@sunshowers sunshowers 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! I think this is ready to go :D

@@ -1,4 +1,4 @@
[![tracing-distributed on crates.io](https://img.shields.io/crates/v/tracing-distributed)](https://crates.io/crates/tracing-distributed) [![Documentation (latest release)](https://docs.rs/tracing-distributed/badge.svg)](https://docs.rs/tracing-distributed/) [![Documentation (master)](https://img.shields.io/badge/docs-master-brightgreen)](https://inanna-malick.github.io/honeycomb-tracing/tracing_distributed/)[![License](https://img.shields.io/badge/license-MIT-green.svg)](../LICENSE)
[![tracing-distributed on crates.io](https://img.shields.io/crates/v/tracing-distributed)](https://crates.io/crates/tracing-distributed) [![Documentation (latest release)](https://docs.rs/tracing-distributed/badge.svg)](https://docs.rs/tracing-distributed/) [![Documentation (master)](https://img.shields.io/badge/docs-master-brightgreen)](https://inanna-malick.github.io/honeycomb-tracing/tracing_distributed/)[![License](https://img.shields.io/badge/license-MIT-green.svg)](../LICENSE)[![inanna-malick](https://circleci.com/gh/inanna-malick/honeycomb-tracing.svg?style=svg)](https://app.circleci.com/pipelines/github/inanna-malick/honeycomb-tracing)

Choose a reason for hiding this comment

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

the alt text inanna-malick should be something like "CircleCI status"

@@ -1,4 +1,4 @@
[![tracing-distributed on crates.io](https://img.shields.io/crates/v/tracing-distributed)](https://crates.io/crates/tracing-distributed) [![Documentation (latest release)](https://docs.rs/tracing-distributed/badge.svg)](https://docs.rs/tracing-distributed/) [![Documentation (master)](https://img.shields.io/badge/docs-master-brightgreen)](https://inanna-malick.github.io/honeycomb-tracing/tracing_distributed/)[![License](https://img.shields.io/badge/license-MIT-green.svg)](../LICENSE)
[![tracing-distributed on crates.io](https://img.shields.io/crates/v/tracing-distributed)](https://crates.io/crates/tracing-distributed) [![Documentation (latest release)](https://docs.rs/tracing-distributed/badge.svg)](https://docs.rs/tracing-distributed/) [![Documentation (master)](https://img.shields.io/badge/docs-master-brightgreen)](https://inanna-malick.github.io/honeycomb-tracing/tracing_distributed/)[![License](https://img.shields.io/badge/license-MIT-green.svg)](../LICENSE)[![inanna-malick](https://circleci.com/gh/inanna-malick/honeycomb-tracing.svg?style=svg)](https://app.circleci.com/pipelines/github/inanna-malick/honeycomb-tracing)

Choose a reason for hiding this comment

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

same here

@@ -1,4 +1,4 @@
[![tracing-honeycomb on crates.io](https://img.shields.io/crates/v/tracing-honeycomb)](https://crates.io/crates/tracing-honeycomb) [![Documentation (latest release)](https://docs.rs/tracing-honeycomb/badge.svg)](https://docs.rs/tracing-honeycomb/) [![Documentation (master)](https://img.shields.io/badge/docs-master-brightgreen)](https://inanna-malick.github.io/honeycomb-tracing/tracing_honeycomb/)[![License](https://img.shields.io/badge/license-MIT-green.svg)](../LICENSE)
[![tracing-honeycomb on crates.io](https://img.shields.io/crates/v/tracing-honeycomb)](https://crates.io/crates/tracing-honeycomb) [![Documentation (latest release)](https://docs.rs/tracing-honeycomb/badge.svg)](https://docs.rs/tracing-honeycomb/) [![Documentation (master)](https://img.shields.io/badge/docs-master-brightgreen)](https://inanna-malick.github.io/honeycomb-tracing/tracing_honeycomb/)[![License](https://img.shields.io/badge/license-MIT-green.svg)](../LICENSE)[![inanna-malick](https://circleci.com/gh/inanna-malick/honeycomb-tracing.svg?style=svg)](https://app.circleci.com/pipelines/github/inanna-malick/honeycomb-tracing)

Choose a reason for hiding this comment

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

same here

@@ -1,4 +1,4 @@
[![tracing-honeycomb on crates.io](https://img.shields.io/crates/v/tracing-honeycomb)](https://crates.io/crates/tracing-honeycomb) [![Documentation (latest release)](https://docs.rs/tracing-honeycomb/badge.svg)](https://docs.rs/tracing-honeycomb/) [![Documentation (master)](https://img.shields.io/badge/docs-master-brightgreen)](https://inanna-malick.github.io/honeycomb-tracing/tracing_honeycomb/)[![License](https://img.shields.io/badge/license-MIT-green.svg)](../LICENSE)
[![tracing-honeycomb on crates.io](https://img.shields.io/crates/v/tracing-honeycomb)](https://crates.io/crates/tracing-honeycomb) [![Documentation (latest release)](https://docs.rs/tracing-honeycomb/badge.svg)](https://docs.rs/tracing-honeycomb/) [![Documentation (master)](https://img.shields.io/badge/docs-master-brightgreen)](https://inanna-malick.github.io/honeycomb-tracing/tracing_honeycomb/)[![License](https://img.shields.io/badge/license-MIT-green.svg)](../LICENSE)[![inanna-malick](https://circleci.com/gh/inanna-malick/honeycomb-tracing.svg?style=svg)](https://app.circleci.com/pipelines/github/inanna-malick/honeycomb-tracing)

Choose a reason for hiding this comment

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

and here

Add the following to your Cargo.toml to get started.

```toml
tracing-honeycomb = 0.1.0-alpha-1

Choose a reason for hiding this comment

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

this may still need to be addressed

Add the following to your Cargo.toml to get started.

```toml
tracing-honeycomb = {{version}}

Choose a reason for hiding this comment

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

I guess this needs to be quoted

@Fishrock123
Copy link
Contributor

Anything holding this up? It seems most comments have been addressed.

I think it would be ideal to get this out first and then iterate on a 0.3 soon from the open PRs.

@inanna-malick
Copy link
Owner Author

This PR can be closed, the main branch is more up to date

@inanna-malick inanna-malick deleted the v2-rain branch December 16, 2020 17:42
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.

3 participants