[MetricsAdvisor] Updated DimensionKey implementation #21972
Conversation
ghost
added
the
kinelski
force-pushed
the
ma-dimensionKey
branch
from
ff72e3b to
cee9518
Compare
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/api/Azure.AI.MetricsAdvisor.netstandard2.0.cs
Outdated
Show resolved
Hide resolved
kinelski
force-pushed
the
ma-dimensionKey
branch
2 times, most recently
from
2d7ce31 to
618fffd
Compare
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/api/Azure.AI.MetricsAdvisor.netstandard2.0.cs
Outdated
Show resolved
Hide resolved
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/api/Azure.AI.MetricsAdvisor.netstandard2.0.cs
Outdated
Show resolved
Hide resolved
kinelski
force-pushed
the
ma-dimensionKey
branch
from
618fffd to
e8979d7
Compare
...csAdvisor/tests/MetricsAdvisorAdministrationClient/AnomalyDetectionConfigurationLiveTests.cs
Show resolved
Hide resolved
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/tests/MetricsAdvisorLiveTestBase.cs
Show resolved
Hide resolved
| public MetricSeriesGroupDetectionCondition() | ||
| /// <param name="seriesGroupKey">The key that identifies the group of time series to which these conditions apply within a metric. A subset of the possible dimensions of the associated data feed must be set.</param> | ||
| /// <exception cref="ArgumentNullException"><paramref name="seriesGroupKey"/> is <c>null</c>.</exception> | ||
| public MetricSeriesGroupDetectionCondition(DimensionKey seriesGroupKey) |
There was a problem hiding this comment.
nit: do you have tests to check that we correctly assert when null?
Same for MetricSingleSeriesDetectionCondition
There was a problem hiding this comment.
We don't, but I think it's common to not add constructor assertion tests to every single model, right? We don't do it in FR either. Let me know if you think otherwise.
|
|
||
| ### New Features | ||
| - `DimensionKey` now implements the `IEnumerable<KeyValuePair<string, string>>` interface. Dimension columns can now be enumerated. | ||
| - Added method `Contains` to `DimensionKey` to check whether or not a dimension column is present. |
There was a problem hiding this comment.
Do you think it is worth showing this in any sample? or the use of the TryGetValue? The names and docstrings are clear enough that I don't think it is needed, but asking just in case
There was a problem hiding this comment.
To tell you the truth, I don't think these methods will be used that much. In general, customers will need to go through every element anyway, so they'll use the enumerator (kudos to KC who suggested making it an IEnumerable). I added these methods just in case someone wants to check a specific element without going through the whole set.
2 participants
Changing most of the
DimensionKeyimplementation after architect feedback:IEquatableanymore. We don't have a strong user scenario in which we think this will be helpful, so removing it for the time being.AddandRemovemethods have been removed.IEnumerablenow, so users can enumerate dimension columns.ContainsandTryGetValuemethods so users can check the contents of the instance without the use of theAsDictionarymethod.Also, added tests for
DimensionKey, updated other tests and some samples.