tracingからAttributesを付与してmetricsを出力できるようにtracing-opentelemetryにPRを送った
現在、FRAIMではOpenTelemetryの導入を進めています。
BackendはRustで書かれており、Applicationから出力するMetricsに関してもOpenTelemetryのMetricsを利用したいと考えています。
既にtracingを導入しているので、tracing-opentelemetryを利用することでApplicationにMetricsを組み込めないか検討していました。
その際に必要な機能があったのでPRをtracing-opentelemetryに送ったところマージしてもらえました。
本記事ではその際に考えたことや学びについて書いていきます。
TL;DR
RustのapplicationでOpenTelemetryのMetricsをtracingから出力するためにtracing-opentelemetryを利用している。
その際にAttributeをMetricsに関連づける機能が必要だったので、tracing-subscriberのPer-Layer Filteringを利用して実装した。
前提
まず前提としてtracing-opentelemetryのMetricsLayerを利用すると、tracingのEventを利用してOpenTelemetryのMetricsを出力することができます。
例えば
let provider = MeterProvider::builder().build();
tracing_subscriber::registry().with(MetricsLayer::new(provider)).init();
tracing::info!(monotonic_counter.foo = 1);
とすると、foo Counterをincrementすることができます。
tracing-opentelemetryのversionは0.20.0です。
その他の関連crateは以下の通りです。
[dependencies]
opentelemetry = "0.20.0"
tracing = "0.1.35"
tracing-subscriber = "0.3.0"
PRで実装した機能の概要
tracingからMetricsを出力する際に、Attributeを付与できないといった制約がありました。
Attributeとは、OpenTelemetryにおけるkey valueのペアです。
例えば
tracing::info!(
monotonic_counter.request_count = 1,
http.route = "/foo",
http.response.status_code = 200,
);
とした場合に、request_count Counterは出力されるのですが、http.route="/foo"やhttp.response.sattus_code=200といった情報をmetricsに付与できませんでした。
この点については、feature requestのissueも上がっており、MetricsLayerにEventのkey valueをmetricsに紐付ける機能がリクエストされていました。
この機能については自分も必要だったので、やってみることにしました。
MetricsLayerの仕組み
まずMetricsLayerの仕組みについてみていきます。
tracing_subscriber::layer::Layer traitを実装して、tracing-subscriberとcomposeすることで、UserはEventの各種lifecycle時に任意の処理を実行することができます。
例えば、on_event()はtracing::info!()等でEventが作成された際に呼ばれます。
tracing/tracing-subscriberの詳細な仕組みについては以前ブログに書いたのでよければ読んでみてください。
MetricsLayerはSpanを処理しないので、以下のようにon_event()のみを実装しています。
impl<S> Layer<S> for MetricsLayer
where
S: Subscriber + for<'span> LookupSpan<'span>,
{
fn on_event(&self, event: &tracing::Event<'_>, _ctx: Context<'_, S>) {
let mut metric_visitor = MetricVisitor {
instruments: &self.instruments,
meter: &self.meter,
};
event.record(&mut metric_visitor);
}
}
source
S: Subscriber + for<'span> LookupSpan<'span>としてLayer<S>でLayerに渡しているSはLayerがtracing_subscriberのRegistryにcomposeされることを表現しており、これによってEvent処理時に今いるSpanの情報を利用できます。が、今回は特に利用しないので気にしなくて大丈夫です。
tracingはEventに格納されたFieldにアクセスする手段としてVisit traitを用意してくれています。
この仕組みにより、tracing::info!(key = "foo")のようにstr型のfieldを使うと、tracing側で、Visit::record_str()を呼んでくれます。
impl<'a> Visit for MetricVisitor<'a> {
fn record_debug(&mut self, _field: &Field, _value: &dyn fmt::Debug) {
// Do nothing
}
fn record_u64(&mut self, field: &Field, value: u64) {
/* ... */
}
fn record_f64(&mut self, field: &Field, value: f64) {
/* ... */
}
fn record_i64(&mut self, field: &Field, value: i64) {
/* ...*/
}
}
MetricVisitorでもこのようにVisitが定義されています。
Metricsは数値なので、record_bool()やrecord_str()は実装されていないこともわかります。
さらに処理を追っていきます。例として、record_i64()をみてみると
const METRIC_PREFIX_MONOTONIC_COUNTER: &str = "monotonic_counter.";
const METRIC_PREFIX_COUNTER: &str = "counter.";
const METRIC_PREFIX_HISTOGRAM: &str = "histogram.";
impl<S> Layer<S> for MetricsLayer
where
S: Subscriber + for<'span> LookupSpan<'span>,
{
fn record_i64(&mut self, field: &Field, value: i64) {
if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_MONOTONIC_COUNTER) {
self.instruments.update_metric(
self.meter,
InstrumentType::CounterU64(value as u64),
metric_name,
);
} else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) {
self.instruments.update_metric(/* ... */);
} else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_HISTOGRAM) {
self.instruments.update_metric(/* ... */);
}
}
}
となっており、field名にMetricsLayerが処理の対象とするprefix(monotonic_counter,counter,histogram)が利用されている場合のみ、self.instruments.update_metric()でmetricsの処理を実施していることがわかりました。
Instrumentsの仕組み
というわけで次にInstrumentsについて見ていきます。Instrumentsは以下のように定義されています。
use opentelemetry::metrics::{Counter, Histogram, Meter, MeterProvider, UpDownCounter};
type MetricsMap<T> = RwLock<HashMap<&'static str, T>>;
#[derive(Default)]
pub(crate) struct Instruments {
u64_counter: MetricsMap<Counter<u64>>,
f64_counter: MetricsMap<Counter<f64>>,
i64_up_down_counter: MetricsMap<UpDownCounter<i64>>,
f64_up_down_counter: MetricsMap<UpDownCounter<f64>>,
u64_histogram: MetricsMap<Histogram<u64>>,
i64_histogram: MetricsMap<Histogram<i64>>,
f64_histogram: MetricsMap<Histogram<f64>>,
}
Metricsの種別(Instrument)ごとにmetrics名とmetricsの実装(Counter等)をHashMapで保持しています。
opentelemetry::metricsから利用している、{Counter, UpDownCounter, Histogram}はmetricsの実装です。
opentelemetry_{api,sdk} crateでmetricsがどう実装されているかについても書きたいのですがかなり複雑なので、今回はふれません。
(以下はMetrics関連の処理を読んでいて、metricsが生成されてからexportされるまでの流れを追う際のメモです)
Metricsの生成からexportまでの流れ
Metrics関連のstructの関係
Counterはメモリに現在の値を保持して、incrementしつつ、定期的にexportしていくだけなのでシンプルかと考えていたのですが思ったより色々なコンポーネントが関与していました。
なので本記事ではCounter::add()したら、Metricsが出力されるくらいの理解でいきます。
use opentelemetry::metrics::Meter;
type MetricsMap<T> = RwLock<HashMap<&'static str, T>>;
impl Instruments {
pub(crate) fn update_metric(
&self,
meter: &Meter,
instrument_type: InstrumentType,
metric_name: &'static str,
) {
fn update_or_insert<T>(
map: &MetricsMap<T>,
name: &'static str,
insert: impl FnOnce() -> T,
update: impl FnOnce(&T),
) {
{
let lock = map.read().unwrap();
if let Some(metric) = lock.get(name) {
update(metric);
return;
}
}
// that metric did not already exist, so we have to acquire a write lock to
// create it.
let mut lock = map.write().unwrap();
// handle the case where the entry was created while we were waiting to
// acquire the write lock
let metric = lock.entry(name).or_insert_with(insert);
update(metric)
}
match instrument_type {
InstrumentType::CounterU64(value) => {
update_or_insert(
&self.u64_counter,
metric_name,
|| meter.u64_counter(metric_name).init(),
|ctr| ctr.add(value, &[]),
);
}
/* ...*/
}
}
}
MetricsLayerがmetricsを処理する際によばれるself.instruments.update_metric()は上記のような処理となっています。概ね以下のことがわかります。
- 毎回
RwLock::read()によるlockが発生する - 初回は
MeterによるInstrument(Counter等)の生成処理が実行される -
Counter::add()する際の第二引数には空sliceが渡されている(今回の変更点に関連する)
この処理を確認したことで、MetricsLayerのdocumentの説明がより理解できます。
例えば
No configuration is needed for this Layer, as it's only responsible for
pushing data out to theopentelemetryfamily of crates.
とあるようにMetricsLayerは特に実質的な処理を行っておらずtracing-opentelemetryというcrate名の通り、tracingとopentelemetryのecosystemをつなぐ役割のみを担っています。
また
# Implementation Details
MetricsLayerholds a set of maps, with each map corresponding to a
type of metric supported by OpenTelemetry. These maps are populated lazily.
The first time that a metric is emitted by the instrumentation, aMetric
instance will be created and added to the corresponding map. This means that
any time a metric is emitted by the instrumentation, one map lookup has to
be performed.
In the future, this can be improved by associating eachMetricinstance to
its callsite, eliminating the need for any maps.
(意訳: MetricLayerはOpenTelemetryでサポートされているmetricの種別ごとにmapを保持している。metricsが出力される際にMetricインスタンスが生成され、これらのmapに追加される。これはmetricsが出力されるたびにmapのlookupが実行されることを意味する。将来的にはMetricインスタンスをcallsiteに紐付けることでmapが必要なくなり改善することができるかもしれない。)
という説明もなるほどと理解できます。
ちなみにcallsiteというのは、tracing::info!() macro呼び出し時にstaticで生成されるmacro呼び出し時の情報を指しています。
というわけで、MetricsLayerがmetricsを出力する方法の概要が理解できました。
機能を追加する上での問題点
まず今回追加したい機能を整理します。
Counterに関する仕様では以下のように定められています。
Counter operations
Add
This API MUST accept the following parameter:
* A numeric increment value.
* Attributes to associate with the increment value.
Users can provide attributes to associate with the increment value, but it is up to their discretion. Therefore, this API MUST be structured to accept a variable number of attributes, including none.
として、Counterをincrementする際にAttributesを渡せなければならないとされています。
これを受けて、RustのOpenTelemetryの実装でも
impl<T> Counter<T> {
/// Records an increment to the counter.
pub fn add(&self, value: T, attributes: &[KeyValue]) {
self.0.add(value, attributes)
}
}
のようにincrement時にattributeとして、KeyValueを渡せるようになっています。
ということで、追加したい機能はMetricsLayerでKeyValueを作って、Counter:add()時に渡せば良さそうです。
そんなに難しくなさそうと思い、数行の変更でいけるのではと思っていました。
Visitorパターンと相性が悪い
着手してわかったことですが、Counter::add()呼び出し時にmetrics(counter.foo)以外のfieldをKeyValueに変換して渡すというのはVisitorパターンと相性が悪いということでした。
というのも、例えば以下のようなEventを考えます。
tracing::info!(monotonic_counter.foo = 1, bar = "baz", qux = 2)
このEventを処理する際、さきほどみたMetricVisitorはmonotonic_counter.fooを処理する際はまだ、bar,qux fieldを処理していないので、Counter::add()を呼び出せないということです。
そのため、visit時に対象のfieldが処理対象なら処理するのではなく、一度、すべてのfieldのvisitを完了させたのちに、Counter::add()を呼び出す必要があります。
そうなると、visit時にVec等にmetricsの情報や変換したKeyValueを保持する必要があります。
しかしながら、そうしてしまうと以下のようにmetricsを含まないEventを処理する際に問題があります。
tracing::info!(
http.route = "/foo",
http.response.status_code = 200,
"Handle request"
);
Eventとしては、metricsを含まない方が多いのが自然ですが、MetricsLayerとしては、visitしている途中ではそのEventにmetricsが含まれているかわからないので、Vecの確保やpush等を行う必要があります。
(まずvisitしたのち、metricsが含まれている場合はもう一度visitする方法も考えられますが非効率)
Event初期化時に判定する
問題としては、あるLayerは特定のEventにのみ関心があり、その判定をEvent処理時ではなく、いずれかの初期化時に一度だけ行いたいという状況です。
これは特定の機能を提供するLayerとしては一般に必要になりそうなので、Layer traitにそういったmethodがないかみていたところ、それらしきものがありました。
fn register_callsite(&self, metadata: &'static Metadata<'static>) -> Interest
Registers a new callsite with this layer, returning whether or not the layer is interested in being notified about the callsite, similarly to Subscriber::register_callsite.
とあるので、tracing::info!()を呼び出すと一度だけ、MetricsLayer::register_callsite()を呼んでくれそうです。
またMetadata::callsite() -> Identiriferから、Eventの識別子も取得できるのでEventがmetricsかどうか事前に一度だけ判定したいという機能は実現できそうに思われました。
もっとも、注意が必要なのは
Note: This method (and Layer::enabled) determine whether a span or event is globally enabled, not whether the individual layer will be notified about that span or event. This is intended to be used by layers that implement filtering for the entire stack. Layers which do not wish to be notified about certain spans or events but do not wish to globally disable them should ignore those spans or events in their on_event, on_enter, on_exit, and other notification methods.
(意訳: このメソッドはspanやeventがグローバルで有効かを判定するもので、レイヤー単位でspanやeventの通知を制御するものではない。これはスタック全体のフィルタリングを実装するレイヤーを意図したもの。特定のspanやeventを処理の対象とはしないが、グローバルで無効にしたくないレイヤーは単にon_eventでそれらを無視すればよい。)
とあるように、register_callsite()を利用しても、MetricsLayerでmetrics以外のeventでon_event()が呼ばれなくなるわけではないということです。(そのような仕組みは別であり、のちほど言及します。)
この仕組みはEventやSpanを特定のLevelでfilterする(DEBUGを無視する等) LevelFilter向けであると思われます。
ということで、あるEventがmetricsを含んでおり処理対象かどうかは自前で管理する必要がありそうということがわかりました。
こうした背景から、最初のPRでは以下のように、callsiteごとの判定結果をRwLockで保持する実装を行いました。
use tracing::callsite::Identifier;
struct Callsites {
callsites: RwLock<HashMap<Identifier, CallsiteInfo>>,
}
#[derive(Default)]
struct CallsiteInfo {
has_metrics_fields: bool,
}
impl Callsites {
fn new() -> Self {
Callsites {
callsites: Default::default(),
}
}
fn register_metrics_callsite(&self, callsite: Identifier, info: CallsiteInfo) {
self.callsites.write().unwrap().insert(callsite, info);
}
fn has_metrics_field(&self, callsite: &Identifier) -> bool {
self.callsites
.read()
.unwrap()
.get(callsite)
.map(|info| info.has_metrics_fields)
.unwrap_or(false)
}
}
impl<S> Layer<S> for MetricsLayer
where
S: Subscriber + for<'span> LookupSpan<'span>,
{
fn register_callsite(&self, metadata: &'static tracing::Metadata<'static>) -> Interest {
if metadata.is_event() && self.has_metrics_fields(metadata.fields()) {
self.callsites.register_metrics_callsite(
metadata.callsite(),
CallsiteInfo {
has_metrics_fields: true,
},
);
}
Interest::always()
}
fn on_event(&self, event: &tracing::Event<'_>, _ctx: Context<'_, S>) {
if self
.callsites
.has_metrics_field(&event.metadata().callsite())
{/* ...*/ }
}
}
自分としてもEvent毎にRwLocl::read()が走る実装は厳しいだろうなと思いつつ、レビューをお願いしましたが、やはり別の方法を考える必要がありました。
jtescherさんはtracing-opentelemetryのmaintainer
Per-Layer Filtering
なにか良い方法がないかと思いLayerのdocを読んでいると、Per-Layer Filteringというものを見つけました。
Sometimes, it may be desirable for one Layer to record a particular subset of spans and events, while a different subset of spans and events are recorded by other Layers.
(意訳: 時に、あるレイヤーで特定のspanやeventsのみを処理しつつ、他のレイヤーには影響をあたえたくない場合がある)
まさに、今この状況なのでこの機能使えるのではと思いました。
さらにこの機能はtracing-subscriberのregistry featureが必要なのですがtracing-opentelemetryは既にこのfeatureに依存しているので、breaking changeともならなそうでした。
Metricsを含むEventに限定できれば、on_event()の処理開始時にFieldにmetricsが含まれていることがわかるので、visit時にそれぞれの値を保持しておくアプローチがとれるので、この機能を組み込んでみようと思いました。
FilterとFiltered
まず考えたのが、MetricsLayerにFilter traitを実装するということでした。
しかしながら、docを読んだり手元で動かしてみたりしてわかったのですが、あるLayerにFilterを実装して、tracing subscriberにcomposeしても意図した効果は得られないということでした。
というのも、tracing subscriberへのcomposeは実装的には、Layeredという型に変換されてそれらが全体として、tracingのSubscriber traitを実装するという形になっているのですが、その実装の中で、Filter traitは特に参照されていません。
tracing-subscriberのAPI設計的に、Filterの実装を渡して、FilteredというLayerを使う必要があることがわかりました。
それがなぜかといいますと
thread_local! {
pub(crate) static FILTERING: FilterState = FilterState::new();
}
impl<S, L, F> Layer<S> for Filtered<L, F, S>
where
S: Subscriber + for<'span> registry::LookupSpan<'span> + 'static,
F: layer::Filter<S> + 'static,
L: Layer<S>,
{
fn enabled(&self, metadata: &Metadata<'_>, cx: Context<'_, S>) -> bool {
let cx = cx.with_filter(self.id());
let enabled = self.filter.enabled(metadata, &cx);
FILTERING.with(|filtering| filtering.set(self.id(), enabled));
if enabled {
// If the filter enabled this metadata, ask the wrapped layer if
// _it_ wants it --- it might have a global filter.
self.layer.enabled(metadata, cx)
} else {
// Otherwise, return `true`. The _per-layer_ filter disabled this
// metadata, but returning `false` in `Layer::enabled` will
// short-circuit and globally disable the span or event. This is
// *not* what we want for per-layer filters, as other layers may
// still want this event. Returning `true` here means we'll continue
// asking the next layer in the stack.
//
// Once all per-layer filters have been evaluated, the `Registry`
// at the root of the stack will return `false` from its `enabled`
// method if *every* per-layer filter disabled this metadata.
// Otherwise, the individual per-layer filters will skip the next
// `new_span` or `on_event` call for their layer if *they* disabled
// the span or event, but it was not globally disabled.
true
}
}
fn on_event(&self, event: &Event<'_>, cx: Context<'_, S>) {
self.did_enable(|| {
self.layer.on_event(event, cx.with_filter(self.id()));
})
}
}
}
impl<L, F, S> Filtered<L, F, S> {
fn did_enable(&self, f: impl FnOnce()) {
FILTERING.with(|filtering| filtering.did_enable(self.id(), f))
}
}
あまり実装の詳細には踏み込みませんが、概ね以下の点がわかります。
-
FilteredはLayerを実装しているので、Layerとして振る舞う -
enabled()では例え、wrapしているFilterの実装がfalseを返しても戻り値としてはtrueを返す(Globalで無効にならない) - Thread localとはいえGlobalに値を保持している(
FILTERING) -
on_event()実装時にGlobalのFILTERINGを参照してPer Filter機能を実現
と中々hack的な実装となっており、自前でFilterを実装するだけではだめで、あくまでFilteredを使わないといけないということがわかりました。
ということで、MetricsLayerからFilteredを利用することが次の目標です。
Filteredの生成についてはtracing-subscriberがwith_filter()を用意してくれているのですが、問題は返り値がFilteredということです。
どういうことかといいますと、今やりたいことは、Filtered<MetricsLayer,F,S>という型をtracing-opentelemetryのuserに返したいということなのですが
tracing-opentelemetryとしてはMetricsLayer::new()がpublicなAPIなのでここを変えてしまうと破壊的変更となってしまうことです。
pub type MetricLayer<S> = Filtered<MetricLayerInner,Filter,S>`
上記のようにaliasを使うかも考えたのですが、これにも問題があります。たとえば将来的に、MetricsLayerに追加の設定が必要となり
let layer = MetricsLayer::new(meter_provider).with_foo();
のようにwith_で設定できるAPIが必要になった場合、Filteredは外部の型なので、methodは追加できないということです。
ここで思ったのが、実体としてはtracing-subscriberのFilteredなのだけど、型としてはuserにそれをみせたくないという状況どこかでみたことあるということでした。
そうです、tracing-subscriberのSubscriberです。
pub struct Subscriber<
N = format::DefaultFields,
E = format::Format<format::Full>,
F = LevelFilter,
W = fn() -> io::Stdout,
> {
inner: layer::Layered<F, Formatter<N, E, W>>,
}
Genericsは無視して、着目したいのが、実体としては、Layeredなのですが、それをinnerとしてwrapした型をuserにみせているということです。
この時初めて、tracing-subscriberがどうしてこうのように実装しているかの気持ちがわかりました(勝手に)。
ということで、MetricsLayerにFilteredを組み込んだ結果以下のような実装となりました。
pub struct MetricsLayer<S> {
inner: Filtered<InstrumentLayer, MetricsFilter, S>,
}
形としては同じです。今までのMetricsLayerの責務はInstrumentLayerが担い、FilterはMetricsFilterに実装する形にしました。
デメリットとして、すべてinnerに移譲する形で、Layerを実装する必要がありますが、Subscriberもそうしていたので受け入れることにしました。
これで、破壊的変更を行うことなく、Per-Layer Filteringの機能を取り込むことができました。
Allocationも避けたい
ここまでで、機能的には目標を達成できたのですが、1点不満がありました。
それは以下のように、Event visit前に、Vecのallocationが発生してしまう点です。
impl<S> Layer<S> for InstrumentLayer
where
S: Subscriber + for<'span> LookupSpan<'span>,
{
fn on_event(&self, event: &tracing::Event<'_>, _ctx: Context<'_, S>) {
let mut attributes = Vec::new(); // 👈 ココ
let mut visited_metrics = Vec::new(); // 👈
let mut metric_visitor = MetricVisitor {
attributes: &mut attributes,
visited_metrics: &mut visited_metrics,
};
event.record(&mut metric_visitor);
// associate attrivutes with visited metrics
visited_metrics
.into_iter()
.for_each(|(metric_name, value)| {
self.instruments.update_metric(
&self.meter,
value,
metric_name,
attributes.as_slice(),
);
})
}
}
Vecが必要なのは、tracing::info!()の中にmetricsと他のfieldがいくつあるかわからないからです。例えば、以下のような入力は可能です。
tracing::info!(
counter.foo = 1,
counter.bar = 2,
abc = "abc",
xyz = 123,
);
一応、tracing側で、最大field数の制限(32)があるのですが、その分のArrayを確保するのもresourceの効率的な利用の観点から後退してしまうように思われました。また最大field数が増える可能性もあります。
ここでの問題は、多くの場合、高々数個だが、例外に対応できるように個数制限は設けられないという状態です。
こういうケースにはまるものないかなと調べていてみつけたのがsmallvec crateです。
smallvecの説明には
Small vectors in various sizes. These store a certain number of elements inline, and fall back to the heap for larger allocations. This can be a useful optimization for improving cache locality and reducing allocator traffic for workloads that fit within the inline buffer.
とあるので、自分の理解が正しければ、指定の数まではstack上に確保され、それを超えた場合のみ、通常のVecのようにheapのallocationが発生するというものです。
ということで、SmallVecをvisit時に利用することにしました。
pub(crate) struct MetricVisitor<'a> {
attributes: &'a mut SmallVec<[KeyValue; 8]>,
visited_metrics: &'a mut SmallVec<[(&'static str, InstrumentType); 2]>,
}
fn on_event(&self, event: &tracing::Event<'_>, _ctx: Context<'_, S>) {
let mut attributes = SmallVec::new();
let mut visited_metrics = SmallVec::new();
let mut metric_visitor = MetricVisitor {
attributes: &mut attributes,
visited_metrics: &mut visited_metrics,
};
event.record(&mut metric_visitor);
/* ...*/
}
このあとテストを追加し、無事レビューが通りました。
Benchmark
trace用のlayerのbenchmarkはあったのですがMetricsLayerのbenchmarkはなかったので、追加しました。
本来は旧実装と比較したかったのですが、benchmarkするには、なんらかの形で旧実装を公開する必要があったので断念しました。
こういうときどうすればいいんですかね?
Flamegraph
criterionのbenchmarkにpprofを設定できるのは知りませんでした。利用してみたところ以下のような結果を得られました。
Metrics更新時のlock処理に時間を使っていることがわかります。
まとめ
tracing-opentelemetryにPRを出してみた際考えたことを書いてみました。
Per-Layer Filteringの仕組みを知れたことや、OpenTelemetryのmetrics関連の実装についての学びがありました。
今後もOpenTelemetryやtracing ecosystemへの理解を深められればと思っております。
Discussion