Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@bkchr
Copy link
Member

@bkchr bkchr commented Oct 27, 2020

Based on: #1795

coriolinus and others added 30 commits October 8, 2020 17:05
Unfortunately, this fails to compile right now due to an upstream
failure to compile which is probably brought on by a recent upgrade
to rustc v1.47.
It's not straightforwardly obvious that this is the best way to handle
the case when there is no authority discovery service, but it seems
to be the best option available at the moment.
for debugging purposes, added this to node/subsystem-util/src/lib.rs:472-476:

```rust
Some(registry) => Self::try_register(registry).map_err(|err| {
	eprintln!("PrometheusError calling {}::register: {:?}", std::any::type_name::<Self>(), err);
	err
}),
```

That pointed out where the registration was failing, which led to
this fix. The test still doesn't pass, but it now fails in a new
and different way!
detailed logging determined that using the `Box::new` style of
future generation, the `self.run` method was never being called,
leading to dropped receivers / closed senders for those subsystems,
causing the overseer to shut down immediately.

This is not the final fix needed to get things working properly,
but it's a good start.
Prometheus lets us register simple counters, which aren't very
interesting. It also allows us to register CounterVecs, which are.
With a CounterVec, you can provide a set of labels, which can
later be used to filter the counts.

We were using them wrong, though. This pattern was repeated in a
variety of places in the code:

```rust
// panics with an cardinality mismatch
let my_counter = register(CounterVec::new(opts, &["succeeded", "failed"])?, registry)?;
my_counter.with_label_values(&["succeeded"]).inc()
```

The problem is that the labels provided in the constructor are not
the set of legal values which can be annotated, but a set of individual
label names which can have individual, arbitrary values.

This commit fixes that.
@coriolinus coriolinus linked an issue Oct 28, 2020 that may be closed by this pull request
@montekki
Copy link
Contributor

a CollateOn message has to be sent to the collator protocol in order to properly init it, so there is a little patch i've been testing this branch with today.

Copy link
Contributor

@montekki montekki left a comment

Choose a reason for hiding this comment

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

A README.md in the adder dir describing how to run the whole setup would be super useful.

@rphmeier
Copy link
Contributor

bot merge

@ghost
Copy link

ghost commented Oct 31, 2020

Trying merge.

@ghost ghost merged commit 8cadebb into master Oct 31, 2020
@ghost ghost deleted the bkchr-adder-collator branch October 31, 2020 17:01
ordian added a commit that referenced this pull request Nov 2, 2020
* master:
  Adder collator improvements (#1896)
  Fixes bug that collator wasn't sending `Declare` message (#1895)
  fix service build: enable notifications protocol only under real overseer (#1894)
  Adds test parachain adder collator  (#1864)
  A real overseer feature (#1892)
  Implementer's guide: Approval Voting Subsystem (#1691)
  Companion for #6912 (#1784)
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

migrate adder-collator to v1

8 participants