nlopes / libhoney-rust Goto Github PK

Currently, none of the code samples work right out of the box.

Rust offers two features that can be easily leveraged to ensure documentation is on-par with the current working changes:

1. Implement examples as doc-tests
   Not much explanation needed here. Doc-tests can be set to compile only, or compile and run, depending on what the code does you'd want to choose between the two.

2. Implement examples as integration tests
   This is a bit harder but will test that the library functionality stays working across branches. Integration tests can serve as excellent examples, as they touch upon most, if not all, of the functionality implemented by a library.

Both tests will need to run as a mandatory step in CI, so any braking changes can be fixed before getting merged.

I understand that this project is super-WIP, but having working documentation is crucial for on-boarding people to a library instead of driving them away

Dependabot can't resolve your Rust dependency files.

As a result, Dependabot couldn't update your dependencies.

The error Dependabot encountered was:

    Updating crates.io index
error: failed to select a version for the requirement `log = "= 0.4.10"`
  candidate versions found which didn't match: 0.4.8, 0.4.7, 0.4.6, ...
  location searched: crates.io index
required by package `libhoney-rust v0.1.3 (/home/dependabot/dependabot-updater/dependabot_tmp_dir

If you think the above is an error on Dependabot's side please don't hesitate to get in touch - we'll do whatever we can to fix it.

View the update logs.

Per https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries I see you've not got a Cargo.lock, but we're using the = requirements specifier for all our dependencies, which can still cause problems.

My specific issue is that the new 2.0.0 release of actix-web depends on a strictly later version of serde, meaning that my project can't now contain both the new release of actix-web and libhoney-rust.

Can we relax the dependencies to caret dependencies, and trust semver to do the right thing unless proven otherwise?

running 15 tests
test builder::tests::test_builder_add ... ok
test builder::tests::test_builder_add_conflict ... ok
test client::tests::test_flush ... FAILED
test client::tests::test_init ... ok
test client::tests::test_send_without_api_key ... ok
test event::tests::test_add ... ok
test event::tests::test_empty ... ok
test event::tests::test_send ... FAILED
test tests::test_init ... ok
test transmission::tests::test_bad_response ... FAILED
test transmission::tests::test_defaults ... ok
test transmission::tests::test_metadata ... FAILED
test transmission::tests::test_modifiable_defaults ... ok
test transmission::tests::test_multiple_batches ... FAILED
test transmission::tests::test_responses ... FAILED

failures:

---- client::tests::test_flush stdout ----
thread 'main' panicked at 'assertion failed: (left == right)
left: None,
right: Some(202)', src/client.rs:197:9
note: Run with RUST_BACKTRACE=1 environment variable to display a backtrace.

---- event::tests::test_send stdout ----
thread 'main' panicked at 'assertion failed: (left == right)
left: None,
right: Some(200)', src/event.rs:216:13

---- transmission::tests::test_bad_response stdout ----
thread 'main' panicked at 'assertion failed: (left == right)
left: None,
right: Some(400)', src/transmission.rs:620:13

---- transmission::tests::test_metadata stdout ----
thread 'main' panicked at 'assertion failed: (left == right)
left: None,
right: Some(202)', src/transmission.rs:513:13

---- transmission::tests::test_multiple_batches stdout ----
thread 'main' panicked at 'assertion failed: (left == right)
left: None,
right: Some(202)', src/transmission.rs:578:9

---- transmission::tests::test_responses stdout ----
thread 'main' panicked at 'assertion failed: (left == right)
left: None,
right: Some(202)', src/transmission.rs:470:13

failures:
client::tests::test_flush
event::tests::test_send
transmission::tests::test_bad_response
transmission::tests::test_metadata
transmission::tests::test_multiple_batches
transmission::tests::test_responses

test result: FAILED. 9 passed; 6 failed; 0 ignored; 0 measured; 0 filtered out

I was following the issue here eaze/tracing-honeycomb#9, and it seems that some environments are having issues with this library (I myself am not one of these currently). There is a fork of this library that uses async channels (https://github.com/agrinman/libhoney-rust) and this has resolved those issues for some.

Additionally, it just seems like the more cohesive approach would be to use async channels because this library is built on async anyway.

Latest version released is 0.1.6, but this version change is still stuck at reqwest 0.10.10 which itself is stick at tokio 0.2 which is incompatible with current tokio versions.

This repo's Cargo.toml has been updated to newer reqwest, but the version on crates.io does not reflect this.

Dependabot can't resolve your Rust dependency files.

As a result, Dependabot couldn't update your dependencies.

The error Dependabot encountered was:

    Updating crates.io index
error: failed to select a version for the requirement `log = "= 0.4.10"`
  candidate versions found which didn't match: 0.4.8, 0.4.7, 0.4.6, ...
  location searched: crates.io index
required by package `libhoney-rust v0.1.3 (/home/dependabot/dependabot-updater/dependabot_tmp_dir

If you think the above is an error on Dependabot's side please don't hesitate to get in touch - we'll do whatever we can to fix it.

View the update logs.

Dependabot can't resolve your Rust dependency files.

As a result, Dependabot couldn't update your dependencies.

The error Dependabot encountered was:

    Updating crates.io index
error: failed to select a version for the requirement `log = ">= 0.4.10"`
  candidate versions found which didn't match: 0.4.8, 0.4.7, 0.4.6, ...
  location searched: crates.io index
required by package `libhoney-rust v0.1.3 (/home/dependabot/dependabot-updater/dependabot_tmp_dir

If you think the above is an error on Dependabot's side please don't hesitate to get in touch - we'll do whatever we can to fix it.

View the update logs.

Description

We noticed an issue where the response channel will fill up and freeze the transmission if responses are not consumed.

A send call is used to send information to the response channel. It seems that the response channel is a bounded crossbeam-channel, so the sender's send function will block if the channel is full.

With a default sized channel, this occurs after 40000 events.

Meanwhile, the README states:

You don’t have to process responses if you’re not interested in them—simply ignoring them is perfectly safe. Unread responses will be dropped.

Code

History

The bug seems to originate in this commit: 2f004da

Potential ideas

I would propose a solution where everything regarding the responses is put behind a feature flag and then explicitly state that you must consume the response channel if you activate the feature.

I would be happy to implement a fix.
Just let me know if you think the proposed idea would be a good fit for this project.

The current client flush interface causes an asynchronous flush:

That function returns once the stop event has been queued and the new worker has been spawned. It would be useful to have this interface (or another) return a future that callers can wait on in order to determine when the original queue has been emptied.

An example use case is for a serverless (AWS Lambda) function, where we need to flush the events to Honeycomb before the Lambda runtime suspends the execution environment until the next function invocation.

A straightforward solution would be for stop to create a oneshot channel and pass the sender into the stop_event, and return a future that waits on the receiver. The stop event handler can then send to the channel when it's done flushing. I'd be happy to contribute a PR to do this (I need it pretty badly for a project) if that approach seems reasonable or if there's another approach you'd prefer!

The client flush method is implemented in terms of stopping and restarting the Transmission:

Unfortunately, the stop event causes a break that breaks out of the loop and doesn't send any of the queued events:

I think the code should probably jump to this line instead of breaking out of the loop: