Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

Initial pass at Metrics support in ILogger #709

Closed
wants to merge 9 commits into from
Closed

Conversation

analogrelay
Copy link
Contributor

This implements #708. It's got some naive elements, like the way we handle filters (which involves some locking). I want to clean that up but wanted to get this into a PR first.

@@ -136,6 +139,21 @@ public IDisposable BeginScope<TState>(TState state)
return scope;
}

public IMetric DefineMetric(string name)
{
if (!_metrics.TryGetValue(name, out var metric))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is dictionary safe for multi thread read during the write?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm honestly not sure. I was having the same concern and thinking I should just use ConcurrentDictionary

@@ -13,6 +13,7 @@
<ItemGroup>
<PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" />
<PackageReference Include="Microsoft.Extensions.Options" />
<PackageReference Include="System.Collections.Immutable" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not anymore, will remove

_name = name;
}

public void RecordValue(double value)
Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed this implementation has to be optimized to update metrics array only when required.

}
}

public void RecordValue<T>(double value, T properties) where T : IEnumerable<KeyValuePair<string, object>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Test for this and TimeSpan extension method.

@analogrelay
Copy link
Contributor Author

Closing this, we're going to look at using EventCounters for our metrics needs.

@pakrym pakrym deleted the anurse/metrics branch November 6, 2018 22:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants