OLD - Custom Timeline Events #302

Closed
wants to merge 51 commits into
from

Conversation

Projects
None yet
8 participants
@nikmd23
Member

nikmd23 commented Aug 2, 2013

As a developer using Glimpse (not an extensions/tab author), I want to make it easier for me to register custom entries in the timeline.

This thread was originally a issue and was converted to a pull request

Currently, if I want to register an event, I have to somehow get an instance of the messages bus, create a custom class create own stopwatch or get a hold of the registered Func<IExecutionTimer> and then profile the desired code. All of this take a lot more lines of code than it should.

This should be a one liner for moment in time events or a simple using statement if its a duration event. Any solution probably should be able to be accessed statically and a null provider registered in this hook so that the code works for testing, but when my code fully runs, it uses a real provider.

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Apr 4, 2013

Member

Yep, this has always been a feature we wanted to add back in. (It existed in the betas, but never made it across the finish line.)

Member

nikmd23 commented Apr 4, 2013

Yep, this has always been a feature we wanted to add back in. (It existed in the betas, but never made it across the finish line.)

@digiguru

This comment has been minimized.

Show comment
Hide comment
@digiguru

digiguru May 8, 2013

Can you explain how you do it with a code sample in the meantime, please?

digiguru commented May 8, 2013

Can you explain how you do it with a code sample in the meantime, please?

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 May 8, 2013

Member

@digiguru,

Right now you do it by publishing a message that implements ITimelineMessage to the message broker.

This is relatively easy to do from an implementation of IInspector, but downright difficult from some random custom code somewhere in your app.

Member

nikmd23 commented May 8, 2013

@digiguru,

Right now you do it by publishing a message that implements ITimelineMessage to the message broker.

This is relatively easy to do from an implementation of IInspector, but downright difficult from some random custom code somewhere in your app.

@digiguru

This comment has been minimized.

Show comment
Hide comment
@digiguru

digiguru May 9, 2013

@nikmd23,

In that case, is there a way I can create a new tab that gets the Timeline view? I have created my own module that returns a TimelineModel. How do I make the TimelineModel get the timeline view applied?

digiguru commented May 9, 2013

@nikmd23,

In that case, is there a way I can create a new tab that gets the Timeline view? I have created my own module that returns a TimelineModel. How do I make the TimelineModel get the timeline view applied?

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 May 13, 2013

Member

To get the timeline view to render on your data, you'll have to leverage the client side extensibility model as well. (Timeline tab has a special renderer.)

There isn't any documentation on the client side API yet, but perhaps @avanderhoorn would be nice enough to give you some pointers here?

Member

nikmd23 commented May 13, 2013

To get the timeline view to render on your data, you'll have to leverage the client side extensibility model as well. (Timeline tab has a special renderer.)

There isn't any documentation on the client side API yet, but perhaps @avanderhoorn would be nice enough to give you some pointers here?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn May 13, 2013

Member

@digiguru I never thought anyone else would want to use the timeline view, so its not publicly exposed as a rendering possibility. If I can ask what sort of timing infor you wanting to show?

Member

avanderhoorn commented May 13, 2013

@digiguru I never thought anyone else would want to use the timeline view, so its not publicly exposed as a rendering possibility. If I can ask what sort of timing infor you wanting to show?

@digiguru

This comment has been minimized.

Show comment
Hide comment
@digiguru

digiguru May 13, 2013

My web form application has 3 classes to control all the cache, database and external service calls. I have timing info recorded for each of these. 

Originally I wanted to display these in the timeline tab, but can't figure out how to get them into glimpse's timeline. Instead I have created a new tab and mirrored the data being returned in the timeline. Perhaps I can just override the timeline tab on the server with custom code as it essentially empty for web forms?

Sent from Mailbox for iPhone

On Mon, May 13, 2013 at 9:28 PM, Anthony van der Hoorn
notifications@github.com wrote:

@digiguru I never thought anyone else would want to use the timeline view, so its not publicly exposed as a rendering possibility. If I can ask what sort of timing infor you wanting to show?

Reply to this email directly or view it on GitHub:
#302 (comment)

My web form application has 3 classes to control all the cache, database and external service calls. I have timing info recorded for each of these. 

Originally I wanted to display these in the timeline tab, but can't figure out how to get them into glimpse's timeline. Instead I have created a new tab and mirrored the data being returned in the timeline. Perhaps I can just override the timeline tab on the server with custom code as it essentially empty for web forms?

Sent from Mailbox for iPhone

On Mon, May 13, 2013 at 9:28 PM, Anthony van der Hoorn
notifications@github.com wrote:

@digiguru I never thought anyone else would want to use the timeline view, so its not publicly exposed as a rendering possibility. If I can ask what sort of timing infor you wanting to show?

Reply to this email directly or view it on GitHub:
#302 (comment)

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn May 13, 2013

Member

Arr ok, on the same page now. So I have good news and bad news. The good
news is you can definitely add your own events to the timeline. Bad news is
that currently it isn't as easy as we would like it to be - hence why this
issue was created. Making this easier is definitely on our list of todos
but to be honest we could use some help getting it done. Any chance you
would like to pitch in?

On Tue, May 14, 2013 at 6:57 AM, digiguru notifications@github.com wrote:

My web form application has 3 classes to control all the cache, database
and external service calls. I have timing info recorded for each of these.

Originally I wanted to display these in the timeline tab, but can't figure
out how to get them into glimpse's timeline. Instead I have created a new
tab and mirrored the data being returned in the timeline. Perhaps I can
just override the timeline tab on the server with custom code as it
essentially empty for web forms?

Sent from Mailbox for iPhone

On Mon, May 13, 2013 at 9:28 PM, Anthony van der Hoorn
notifications@github.com wrote:

@digiguru I never thought anyone else would want to use the timeline
view, so its not publicly exposed as a rendering possibility. If I can ask

what sort of timing infor you wanting to show?

Reply to this email directly or view it on GitHub:
#302 (comment)


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/issues/302#issuecomment-17840212
.

Member

avanderhoorn commented May 13, 2013

Arr ok, on the same page now. So I have good news and bad news. The good
news is you can definitely add your own events to the timeline. Bad news is
that currently it isn't as easy as we would like it to be - hence why this
issue was created. Making this easier is definitely on our list of todos
but to be honest we could use some help getting it done. Any chance you
would like to pitch in?

On Tue, May 14, 2013 at 6:57 AM, digiguru notifications@github.com wrote:

My web form application has 3 classes to control all the cache, database
and external service calls. I have timing info recorded for each of these.

Originally I wanted to display these in the timeline tab, but can't figure
out how to get them into glimpse's timeline. Instead I have created a new
tab and mirrored the data being returned in the timeline. Perhaps I can
just override the timeline tab on the server with custom code as it
essentially empty for web forms?

Sent from Mailbox for iPhone

On Mon, May 13, 2013 at 9:28 PM, Anthony van der Hoorn
notifications@github.com wrote:

@digiguru I never thought anyone else would want to use the timeline
view, so its not publicly exposed as a rendering possibility. If I can ask

what sort of timing infor you wanting to show?

Reply to this email directly or view it on GitHub:
#302 (comment)


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/issues/302#issuecomment-17840212
.

@digiguru

This comment has been minimized.

Show comment
Hide comment
@digiguru

digiguru May 14, 2013

Sure, I'll try to figure it out using this conversation. 

If you have an example of code that implements iTimelineMessage or that exposes the message bus it would give me an appropriate starting point.

Sent from Mailbox for iPhone

On Mon, May 13, 2013 at 10:19 PM, Anthony van der Hoorn
notifications@github.com wrote:

Arr ok, on the same page now. So I have good news and bad news. The good
news is you can definitely add your own events to the timeline. Bad news is
that currently it isn't as easy as we would like it to be - hence why this
issue was created. Making this easier is definitely on our list of todos
but to be honest we could use some help getting it done. Any chance you
would like to pitch in?
On Tue, May 14, 2013 at 6:57 AM, digiguru notifications@github.com wrote:

My web form application has 3 classes to control all the cache, database
and external service calls. I have timing info recorded for each of these.

Originally I wanted to display these in the timeline tab, but can't figure
out how to get them into glimpse's timeline. Instead I have created a new
tab and mirrored the data being returned in the timeline. Perhaps I can
just override the timeline tab on the server with custom code as it
essentially empty for web forms?

Sent from Mailbox for iPhone

On Mon, May 13, 2013 at 9:28 PM, Anthony van der Hoorn
notifications@github.com wrote:

@digiguru I never thought anyone else would want to use the timeline
view, so its not publicly exposed as a rendering possibility. If I can ask

what sort of timing infor you wanting to show?

Reply to this email directly or view it on GitHub:
#302 (comment)


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/issues/302#issuecomment-17840212
.


Reply to this email directly or view it on GitHub:
#302 (comment)

Sure, I'll try to figure it out using this conversation. 

If you have an example of code that implements iTimelineMessage or that exposes the message bus it would give me an appropriate starting point.

Sent from Mailbox for iPhone

On Mon, May 13, 2013 at 10:19 PM, Anthony van der Hoorn
notifications@github.com wrote:

Arr ok, on the same page now. So I have good news and bad news. The good
news is you can definitely add your own events to the timeline. Bad news is
that currently it isn't as easy as we would like it to be - hence why this
issue was created. Making this easier is definitely on our list of todos
but to be honest we could use some help getting it done. Any chance you
would like to pitch in?
On Tue, May 14, 2013 at 6:57 AM, digiguru notifications@github.com wrote:

My web form application has 3 classes to control all the cache, database
and external service calls. I have timing info recorded for each of these.

Originally I wanted to display these in the timeline tab, but can't figure
out how to get them into glimpse's timeline. Instead I have created a new
tab and mirrored the data being returned in the timeline. Perhaps I can
just override the timeline tab on the server with custom code as it
essentially empty for web forms?

Sent from Mailbox for iPhone

On Mon, May 13, 2013 at 9:28 PM, Anthony van der Hoorn
notifications@github.com wrote:

@digiguru I never thought anyone else would want to use the timeline
view, so its not publicly exposed as a rendering possibility. If I can ask

what sort of timing infor you wanting to show?

Reply to this email directly or view it on GitHub:
#302 (comment)


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/issues/302#issuecomment-17840212
.


Reply to this email directly or view it on GitHub:
#302 (comment)

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn May 16, 2013

Member

At the moment you first need to start out creating a message ITimelineMessage. If you have a look at the file, you will see there is some extension methods that you can use to make the usage easier. Given all of this, I would start off by creating an object that inherits from this interface.

Next, the interface that an end user should use, should look something like the following:

var instance = Glimpse.Runtime.Instance;
using (instance.Capture(...)) {
    //...
}

Note, @nikmd23 may have some ideas on what he wants the exact interface to be, but the general structure will be very similar, its likely just to be name differences.

Given this see Settings as an example of how to go develop using Glimpse.Runtime.Instance.

By calling Instance, it should have an object that internally contains an instance of a MessageBroker object. In terms of getting a hold of the message broker, for the time being you can tap into via the following hook - link.

This object should also have an instance method for Capture (that I image would have a few overloads) that creates an instance of the object you create that inherits from ITimelineMessage and pushes that through the message provider.

Lastly, if you can make sure the work it unit tested that would be amazing!

I know this seems like a little, but I think it should be fairly straightforward and once we have this, everyone will be able to simple add their own events to the timeline. Also we would be eternally grateful for your help.

Member

avanderhoorn commented May 16, 2013

At the moment you first need to start out creating a message ITimelineMessage. If you have a look at the file, you will see there is some extension methods that you can use to make the usage easier. Given all of this, I would start off by creating an object that inherits from this interface.

Next, the interface that an end user should use, should look something like the following:

var instance = Glimpse.Runtime.Instance;
using (instance.Capture(...)) {
    //...
}

Note, @nikmd23 may have some ideas on what he wants the exact interface to be, but the general structure will be very similar, its likely just to be name differences.

Given this see Settings as an example of how to go develop using Glimpse.Runtime.Instance.

By calling Instance, it should have an object that internally contains an instance of a MessageBroker object. In terms of getting a hold of the message broker, for the time being you can tap into via the following hook - link.

This object should also have an instance method for Capture (that I image would have a few overloads) that creates an instance of the object you create that inherits from ITimelineMessage and pushes that through the message provider.

Lastly, if you can make sure the work it unit tested that would be amazing!

I know this seems like a little, but I think it should be fairly straightforward and once we have this, everyone will be able to simple add their own events to the timeline. Also we would be eternally grateful for your help.

@digiguru

This comment has been minimized.

Show comment
Hide comment
@digiguru

digiguru May 22, 2013

Hi, I don't have a lot of free time, but I am going to start looking into this.

I'll get back to you when I have made some progress.

Hi, I don't have a lot of free time, but I am going to start looking into this.

I'll get back to you when I have made some progress.

@mattwarren

This comment has been minimized.

Show comment
Hide comment
@mattwarren

mattwarren Sep 17, 2013

Is there an update on this, will it be implemented or are you waiting for a pull-request?

I'd really like to be able to write timing code like this:

using (Glimpse.Runtime.Instance.Capture("SomeCustomCode")) {
    //...
}

And have the timings for the code inside the using statement show up in the Glimpse timeline/overview timings.

Is there an update on this, will it be implemented or are you waiting for a pull-request?

I'd really like to be able to write timing code like this:

using (Glimpse.Runtime.Instance.Capture("SomeCustomCode")) {
    //...
}

And have the timings for the code inside the using statement show up in the Glimpse timeline/overview timings.

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Sep 20, 2013

Member

No update at the moment @mattwarren. As soon as #266 is completed we'll be able to complete this one pretty easily.

I'm working on #266 as we speak.

Member

nikmd23 commented Sep 20, 2013

No update at the moment @mattwarren. As soon as #266 is completed we'll be able to complete this one pretty easily.

I'm working on #266 as we speak.

@vmachacek

This comment has been minimized.

Show comment
Hide comment
@vmachacek

vmachacek Oct 2, 2013

I just finished implmentation of Glimpse on production and now I need to time some image generations, and pdf creations. I guess, its still not possible? I reverted from MiniProfiler, which has this outside the box. Dont get me wrong its great work. I can wait, no problem...

I just finished implmentation of Glimpse on production and now I need to time some image generations, and pdf creations. I guess, its still not possible? I reverted from MiniProfiler, which has this outside the box. Dont get me wrong its great work. I can wait, no problem...

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Oct 2, 2013

Member

You can get this if you want it. By default glimpse only runs on requests
of particular content types.

See the following for how you can do it -
http://getglimpse.com/Help/Configuration (under "content type"). Note, to
see that data gathered for those requests, you will need to use the history
tab.

Out of curiosity, you mentioned that MP has this feature, how did you
discover that functionality? I'm wanting to improve how we do things if
possible.

On Wednesday, October 2, 2013, Kostkac wrote:

I just finished implmentation of Glimpse on production and now I need to
time some image generations, and pdf creations. I guess, its still not
possible? I reverted from MiniProfiler, which has this outside the box.


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/302#issuecomment-25578542
.

Member

avanderhoorn commented Oct 2, 2013

You can get this if you want it. By default glimpse only runs on requests
of particular content types.

See the following for how you can do it -
http://getglimpse.com/Help/Configuration (under "content type"). Note, to
see that data gathered for those requests, you will need to use the history
tab.

Out of curiosity, you mentioned that MP has this feature, how did you
discover that functionality? I'm wanting to improve how we do things if
possible.

On Wednesday, October 2, 2013, Kostkac wrote:

I just finished implmentation of Glimpse on production and now I need to
time some image generations, and pdf creations. I guess, its still not
possible? I reverted from MiniProfiler, which has this outside the box.


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/302#issuecomment-25578542
.

@vmachacek

This comment has been minimized.

Show comment
Hide comment
@vmachacek

vmachacek Oct 2, 2013

Maybe I didnt express myself corectly, I want to use using statement in the c# code to measure how much time it takes to generate thumbnails after upload. Currently Im doing it synchronously, so It would be visible in the Timeline tab. MiniProfiler does this with technique mentioned here may times. Wrap calling of servise into miniprofiler's using and that will add extra row in results. Btw how does it work when I add MiniProfiler plugin into the Glimpse? Do I need original MiniProfiler?

var profiler = MiniProfiler.Current;

using (profiler.Step("Generating of Thumbnails"))
{
   //Some expensive code  
   _imageService.Generate();
}

Maybe I didnt express myself corectly, I want to use using statement in the c# code to measure how much time it takes to generate thumbnails after upload. Currently Im doing it synchronously, so It would be visible in the Timeline tab. MiniProfiler does this with technique mentioned here may times. Wrap calling of servise into miniprofiler's using and that will add extra row in results. Btw how does it work when I add MiniProfiler plugin into the Glimpse? Do I need original MiniProfiler?

var profiler = MiniProfiler.Current;

using (profiler.Step("Generating of Thumbnails"))
{
   //Some expensive code  
   _imageService.Generate();
}
@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Oct 16, 2013

Member

@Kostkac,

We are not the authors of the MiniProfiler plugin for Glimpse. 😦 - you'll have to ask them about how it works.

Member

nikmd23 commented Oct 16, 2013

@Kostkac,

We are not the authors of the MiniProfiler plugin for Glimpse. 😦 - you'll have to ask them about how it works.

@JSkimming

This comment has been minimized.

Show comment
Hide comment
@JSkimming

JSkimming Oct 17, 2013

@Kostkac,

We've used MiniProfiler and glimpse together, but as soon as the custom timeline feature is released we'll just be using glimpse as it will do all of what MiniProfiler does (and then some).

To answer your question, when last I used it (a few months ago), you do still need the original MiniProfiler, we have found one glitch though (I should raise it as a glimpse issue)

To prevent Glimpse and MiniProfiler monitoring each other's requests, you have to tell them to ignore requests that match a pattern, in Glimpse you add this

<glimpse defaultRuntimePolicy="On" endpointBaseUri="~/Glimpse.axd">
    ... other stuff
    <runtimePolicies>
      <uris>
        <add regex=".*/mini-profiler-resources/.*" />
      </uris>
      ... more stuff
    </runtimePolicies>
    ... even more stuff
</glimpse>

I've found this works fine to ensure glimpse ignores mini profiler except the HUD doesn't seem to honour this runtime policy and still shows the AJAX requests to MiniProfiler.

It's been a few months and it may have been fixed in Glimpse, I'll check (when I next get the chance) and raise an issue if not.

@Kostkac,

We've used MiniProfiler and glimpse together, but as soon as the custom timeline feature is released we'll just be using glimpse as it will do all of what MiniProfiler does (and then some).

To answer your question, when last I used it (a few months ago), you do still need the original MiniProfiler, we have found one glitch though (I should raise it as a glimpse issue)

To prevent Glimpse and MiniProfiler monitoring each other's requests, you have to tell them to ignore requests that match a pattern, in Glimpse you add this

<glimpse defaultRuntimePolicy="On" endpointBaseUri="~/Glimpse.axd">
    ... other stuff
    <runtimePolicies>
      <uris>
        <add regex=".*/mini-profiler-resources/.*" />
      </uris>
      ... more stuff
    </runtimePolicies>
    ... even more stuff
</glimpse>

I've found this works fine to ensure glimpse ignores mini profiler except the HUD doesn't seem to honour this runtime policy and still shows the AJAX requests to MiniProfiler.

It's been a few months and it may have been fixed in Glimpse, I'll check (when I next get the chance) and raise an issue if not.

@droyad

This comment has been minimized.

Show comment
Hide comment
@droyad

droyad Jan 7, 2014

Contributor

Where is this at? I've come up with this based on the suggestions here. I could integrate it into the Glimpse source if you want.

https://gist.github.com/droyad/8292852

Contributor

droyad commented Jan 7, 2014

Where is this at? I've come up with this based on the suggestions here. I could integrate it into the Glimpse source if you want.

https://gist.github.com/droyad/8292852

nikmd23 added some commits Aug 2, 2013

Removed IFrameworkProvider from Configuration - but feel a bit dirty …
…since I commented out a few tests than need deeper review.
Got ASP.NET project working with new Init model.
Skipped a bunch of tests that were hanging the test runner.
Implemented the rest of Glimpse.Owin. Glimpse now operates in OWIN re…
…quests as middleware, but there is a few hacks along the way that need to be cleaned up before this is ready for RC release
Fixed a few broken artifacts of major rebase, including commenting ou…
…t a few tests 😞

There are a few comments with the token V2Merge that need to be reviewed still
@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Jan 16, 2014

Member

I thought about moving this method definition to a separate IProvideConfigurationOverrides interface and letting IGlimpseConfiguration implementers opt-in to allowing for overrides.

However, I decided to keep this as simple as possible since I don't expect very many (any?) other implementations of the IGlimpseConfiguration interface. Plus, other implementations that didn't want to allow for overrides could just leave the method body empty (which yes, is a bit ugly, but like I said will happen seldom if ever.)

I thought about moving this method definition to a separate IProvideConfigurationOverrides interface and letting IGlimpseConfiguration implementers opt-in to allowing for overrides.

However, I decided to keep this as simple as possible since I don't expect very many (any?) other implementations of the IGlimpseConfiguration interface. Plus, other implementations that didn't want to allow for overrides could just leave the method body empty (which yes, is a bit ugly, but like I said will happen seldom if ever.)

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Jan 16, 2014

Member

I've put a rough implementation together. It still needs to be refactored.

The biggest problem right now is the static class Glimpse. Because our namespaces begin with the word Glimpse as well, it is very clumsy to use.

I'm trying to think of a better name for it, feel free to make suggestions. Right now you can do three things with it:

  1. As a user, modify configuration:

    Glimpse.Configuration = config => 
    {
        // modify the configuration
    };
  2. As a user, capture a timing event:

    using(Glimpse.Capture("Some chunk of code to measure"))
    {
        // some custom user code
    }
  3. As a user, capture a moment in time for the timeline:

    Glimpse.CaptureMoment("Some event I want to mark in the timeline");

    Glimpse really does seem like the perfect name for this. I think the users will expect the name to be Glimpse. I've thought about renaming the class to _Glimpse or to break up the functions into two static classes named Timeline and Configuration, which is what I'm currently leaning towards doing.

Member

nikmd23 commented Jan 16, 2014

I've put a rough implementation together. It still needs to be refactored.

The biggest problem right now is the static class Glimpse. Because our namespaces begin with the word Glimpse as well, it is very clumsy to use.

I'm trying to think of a better name for it, feel free to make suggestions. Right now you can do three things with it:

  1. As a user, modify configuration:

    Glimpse.Configuration = config => 
    {
        // modify the configuration
    };
  2. As a user, capture a timing event:

    using(Glimpse.Capture("Some chunk of code to measure"))
    {
        // some custom user code
    }
  3. As a user, capture a moment in time for the timeline:

    Glimpse.CaptureMoment("Some event I want to mark in the timeline");

    Glimpse really does seem like the perfect name for this. I think the users will expect the name to be Glimpse. I've thought about renaming the class to _Glimpse or to break up the functions into two static classes named Timeline and Configuration, which is what I'm currently leaning towards doing.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Jan 21, 2014

Member

Do we think it worth trying to release a version of this that goes out under v1.x? I'm thinking that v2 is still at least a month or two off and we could get wins for people in the mean time.

Member

avanderhoorn commented Jan 21, 2014

Do we think it worth trying to release a version of this that goes out under v1.x? I'm thinking that v2 is still at least a month or two off and we could get wins for people in the mean time.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Jan 22, 2014

Collaborator

Hi guys,

I don't want to be a party pooper, but 😉 what if we would go for a completely different approach, one that adheres more to the Glimpse mindset of being as less invasive as possible for the application.

The current solution means that users will be starting to include Glimpse specific types into their code, just for measuring purposes and to be able to show that afterwards, if at all, in the Glimpse Timeline. This obviously will make it a lot harder for users to exclude Glimpse when the application is prepared for production, without having there code filled with compilation constant checks for instance (which would obviously also mean a recompilation, hence changing the application bits between dev/test/qua/prd which is not always allowed). All of this because the Glimpse assemblies are still needed (at least the Glimpse.Core assembly) at runtime because they are being referenced in code.

So I was wondering, why not leverage the functionality of System.Diagnostics.Trace ? That way users just write Trace statements, which are of course not Glimpse specific, and then, in addition, they only need to configure the Glimpse TraceListener through config.

If we then introduce some conventions like every TraceInformation call starting with :

  • StartTimeCapture : [Message]
  • EndTimeCapture
  • CaptureMoment : [Message]

or even allow those conventions to be configured so that Glimpse is not imposing those defaults, then we can have our Glimpse Tracelistener translate those calls into timeline messages.

Users will have to create there own disposable scope, but that is not a big issue (and we can always show an example on the site) that just makes those TraceInformation() calls

I somehow feel that this is something that can be used anywhere in the code, and users can have there own listeners in case they don't want to / can use Glimpse, or even go without any tracelistener at all, the code would not have to be recompiled in any way then.

What do you think?

Christophe

Collaborator

CGijbels commented Jan 22, 2014

Hi guys,

I don't want to be a party pooper, but 😉 what if we would go for a completely different approach, one that adheres more to the Glimpse mindset of being as less invasive as possible for the application.

The current solution means that users will be starting to include Glimpse specific types into their code, just for measuring purposes and to be able to show that afterwards, if at all, in the Glimpse Timeline. This obviously will make it a lot harder for users to exclude Glimpse when the application is prepared for production, without having there code filled with compilation constant checks for instance (which would obviously also mean a recompilation, hence changing the application bits between dev/test/qua/prd which is not always allowed). All of this because the Glimpse assemblies are still needed (at least the Glimpse.Core assembly) at runtime because they are being referenced in code.

So I was wondering, why not leverage the functionality of System.Diagnostics.Trace ? That way users just write Trace statements, which are of course not Glimpse specific, and then, in addition, they only need to configure the Glimpse TraceListener through config.

If we then introduce some conventions like every TraceInformation call starting with :

  • StartTimeCapture : [Message]
  • EndTimeCapture
  • CaptureMoment : [Message]

or even allow those conventions to be configured so that Glimpse is not imposing those defaults, then we can have our Glimpse Tracelistener translate those calls into timeline messages.

Users will have to create there own disposable scope, but that is not a big issue (and we can always show an example on the site) that just makes those TraceInformation() calls

I somehow feel that this is something that can be used anywhere in the code, and users can have there own listeners in case they don't want to / can use Glimpse, or even go without any tracelistener at all, the code would not have to be recompiled in any way then.

What do you think?

Christophe

@JSkimming

This comment has been minimized.

Show comment
Hide comment
@JSkimming

JSkimming Jan 22, 2014

Just adding to the discussion.

I prefer the using apprach, already added by @nikmd23. While I understand @CGijbels concerns I see custom timeline events similar to extensions, in that it's extending what is out of the box, which requires some level of coupling to glimpse.

Personally, when timeline events are added I'll be making use of it as a cross cutting concern, and won't be liberally adding using(Glimpse.Capture("Some chunk of code to measure")) throughout the code, but at specific places. For instance with a DelegatingHandler to measure use of external services, like Azure Mobile Services, this way the dependency on glimpse is limited to a small set of classes.

I also think there could be pitfalls of a trace logging convention. How could it differentiate multi-threaded and/or asynchronous traces? How could it correlate the ends with the equivalent starts when they are not sequential but overlap?

Just adding to the discussion.

I prefer the using apprach, already added by @nikmd23. While I understand @CGijbels concerns I see custom timeline events similar to extensions, in that it's extending what is out of the box, which requires some level of coupling to glimpse.

Personally, when timeline events are added I'll be making use of it as a cross cutting concern, and won't be liberally adding using(Glimpse.Capture("Some chunk of code to measure")) throughout the code, but at specific places. For instance with a DelegatingHandler to measure use of external services, like Azure Mobile Services, this way the dependency on glimpse is limited to a small set of classes.

I also think there could be pitfalls of a trace logging convention. How could it differentiate multi-threaded and/or asynchronous traces? How could it correlate the ends with the equivalent starts when they are not sequential but overlap?

@JSkimming

This comment has been minimized.

Show comment
Hide comment
@JSkimming

JSkimming Jan 22, 2014

also @avanderhoorn

Do we think it worth trying to release a version of this that goes out under v1.x? I'm thinking that v2 is still at least a month or two off and we could get wins for people in the mean time.

Yes please if possible.

also @avanderhoorn

Do we think it worth trying to release a version of this that goes out under v1.x? I'm thinking that v2 is still at least a month or two off and we could get wins for people in the mean time.

Yes please if possible.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Jan 22, 2014

Collaborator

@JSkimming Thanks for the feedback!

The using approach is indeed what needs to be done, the question is whether or not Glimpse should have that construct in its assemblies or just offer an example. It's like you said, the Glimpse specific stuff will end up being wrapped anyway (at least lets hope so), albeit inside a custom disposable object or a cross cutting concern.

So looking at it as from an extension point of view then the coupling might be OK, but the question remains whether or not everybody will look at it like that?

For a CaptureMoment a trace would work just fine, in case of a timing we indeed need to keep track of any asynchronous processing inside such a using, which would otherwise be stored inside the disposable object.
That could be handled as well by providing a unique correlation id (guid) with the trace call and have that id stored inside the disposable object and reuse it to stop a given capture when disposing, but maybe that is to much? They could use the void TraceInformation(string format, params object[] args) for that and pass in the correlation id?

From looking at previous issues, I've got the feeling that a lot of users want Glimpse to be as less invasive as possible so that they can remove it by simply removing config and assemblies.

Maybe we could do both things, we will need a GlimpseTimeline (or whatever it'll be named) anyway to encapsulate that functionality inside Glimpse, and maybe those who want to use it explicitly can use it, and for those not keen on that explicit use, we might offer a GlimpseTimelineTracingAdapter that allows users to hook into that behavior based on convention based tracing?

Collaborator

CGijbels commented Jan 22, 2014

@JSkimming Thanks for the feedback!

The using approach is indeed what needs to be done, the question is whether or not Glimpse should have that construct in its assemblies or just offer an example. It's like you said, the Glimpse specific stuff will end up being wrapped anyway (at least lets hope so), albeit inside a custom disposable object or a cross cutting concern.

So looking at it as from an extension point of view then the coupling might be OK, but the question remains whether or not everybody will look at it like that?

For a CaptureMoment a trace would work just fine, in case of a timing we indeed need to keep track of any asynchronous processing inside such a using, which would otherwise be stored inside the disposable object.
That could be handled as well by providing a unique correlation id (guid) with the trace call and have that id stored inside the disposable object and reuse it to stop a given capture when disposing, but maybe that is to much? They could use the void TraceInformation(string format, params object[] args) for that and pass in the correlation id?

From looking at previous issues, I've got the feeling that a lot of users want Glimpse to be as less invasive as possible so that they can remove it by simply removing config and assemblies.

Maybe we could do both things, we will need a GlimpseTimeline (or whatever it'll be named) anyway to encapsulate that functionality inside Glimpse, and maybe those who want to use it explicitly can use it, and for those not keen on that explicit use, we might offer a GlimpseTimelineTracingAdapter that allows users to hook into that behavior based on convention based tracing?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Jan 22, 2014

Member

As a side note, we had talked in the past about having a solution which would work for people who don't want to put Glimpse.xyz in their code.

Basically we could just have a file/gist/package that they can get a hold of which has the same API as the underlying Glimpse.xyz, but just proxies the calls. This code wouldn't be rocket science and in terms of delivery, the file would work the same way as most of the micro ORMs do.

The difference with them using this proxy via Glimpse.xyz directly, is that its an independent *.cs file which they can control. If they want to switch out the calls from Glimpse to something else they can very easily. It centralizes the point where they need to go to change things.

Member

avanderhoorn commented Jan 22, 2014

As a side note, we had talked in the past about having a solution which would work for people who don't want to put Glimpse.xyz in their code.

Basically we could just have a file/gist/package that they can get a hold of which has the same API as the underlying Glimpse.xyz, but just proxies the calls. This code wouldn't be rocket science and in terms of delivery, the file would work the same way as most of the micro ORMs do.

The difference with them using this proxy via Glimpse.xyz directly, is that its an independent *.cs file which they can control. If they want to switch out the calls from Glimpse to something else they can very easily. It centralizes the point where they need to go to change things.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Jan 22, 2014

Member

In terms of whether this new Glimpse API simply is a pass through to call System.Diagnostics.Trace and then the timeline looks to try and match up the messages... I guess I don't really mind, but I really hate string processing like that. So unless we had to, it would be good to avoid.

Member

avanderhoorn commented Jan 22, 2014

In terms of whether this new Glimpse API simply is a pass through to call System.Diagnostics.Trace and then the timeline looks to try and match up the messages... I guess I don't really mind, but I really hate string processing like that. So unless we had to, it would be good to avoid.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Jan 22, 2014

Collaborator

I understand, the string matching approach is certainly not the best way, but seemed at first sight the only way to use something existing and hooking into that one.
On the other hand I don't see much of a difference with using proxies or the original stuff, because both options require recompilation of the code to make the switch and be able to not deploy the Glimpse specific assemblies. It seems the same as the approach to wrap the Glimpse.xyz specific code into a custom disposable type and then have the calls to Glimpse conditionally exclude during compilation, which is something we should avoid no?

Maybe somebody could start a new OSS project that tracks timings, for which we could create a Glimpse plugin that integrates with the Glimspe timeline 😉 at least the timings specific code would not be Glimpse specific ;-)

Collaborator

CGijbels commented Jan 22, 2014

I understand, the string matching approach is certainly not the best way, but seemed at first sight the only way to use something existing and hooking into that one.
On the other hand I don't see much of a difference with using proxies or the original stuff, because both options require recompilation of the code to make the switch and be able to not deploy the Glimpse specific assemblies. It seems the same as the approach to wrap the Glimpse.xyz specific code into a custom disposable type and then have the calls to Glimpse conditionally exclude during compilation, which is something we should avoid no?

Maybe somebody could start a new OSS project that tracks timings, for which we could create a Glimpse plugin that integrates with the Glimspe timeline 😉 at least the timings specific code would not be Glimpse specific ;-)

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Jan 22, 2014

Member

In the proxy case, you are right about the recompilation, but the
difference is that Glimpse references aren't littered through their code
base.

On Wed, Jan 22, 2014 at 1:20 PM, Christophe Gijbels <
notifications@github.com> wrote:

I understand, the string matching approach is certainly not the best way,
but seemed at first sight the only way to use something existing and
hooking into that one.
On the other hand I don't see much of a difference with using proxies or
the original stuff, because both options require recompilation of the code
to make the switch and be able to not deploy the Glimpse specific
assemblies. It seems the same as the approach to wrap the Glimpse.xyz
specific code into a custom disposable type and then have the calls to
Glimpse conditionally exclude during compilation, which is something we
should avoid no?

Maybe somebody could start a new OSS project that tracks timings, for
which we could create a Glimpse plugin that integrates with the Glimspe
timeline [image: 😉] at least the timings specific code would not be
Glimpse specific ;-)


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/302#issuecomment-33051220
.

Member

avanderhoorn commented Jan 22, 2014

In the proxy case, you are right about the recompilation, but the
difference is that Glimpse references aren't littered through their code
base.

On Wed, Jan 22, 2014 at 1:20 PM, Christophe Gijbels <
notifications@github.com> wrote:

I understand, the string matching approach is certainly not the best way,
but seemed at first sight the only way to use something existing and
hooking into that one.
On the other hand I don't see much of a difference with using proxies or
the original stuff, because both options require recompilation of the code
to make the switch and be able to not deploy the Glimpse specific
assemblies. It seems the same as the approach to wrap the Glimpse.xyz
specific code into a custom disposable type and then have the calls to
Glimpse conditionally exclude during compilation, which is something we
should avoid no?

Maybe somebody could start a new OSS project that tracks timings, for
which we could create a Glimpse plugin that integrates with the Glimspe
timeline [image: 😉] at least the timings specific code would not be
Glimpse specific ;-)


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/302#issuecomment-33051220
.

@JSkimming

This comment has been minimized.

Show comment
Hide comment
@JSkimming

JSkimming Jan 22, 2014

If I, as a developer who uses Glimpse, have the ability to add custom time line events, then it is my resposability to keep those dependencies on Glimpse to Minimum should I desire it. If I want a trace logging convention that results in timeline events, then I'll implement one, but I don't expect one as part of the glimpse core components.

If I, as a developer who uses Glimpse, have the ability to add custom time line events, then it is my resposability to keep those dependencies on Glimpse to Minimum should I desire it. If I want a trace logging convention that results in timeline events, then I'll implement one, but I don't expect one as part of the glimpse core components.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Jan 22, 2014

Member

Great feedback!

On Wed, Jan 22, 2014 at 1:47 PM, James Skimming notifications@github.comwrote:

If I, as a developer who uses Glimpse, have the ability to add custom time
line events, then it is my resposability to keep those dependencies on
Glimpse to Minimum should I desire it. If I want a trace logging convention
that results in timeline events, then I'll implement one, but I don't
expect one as part of the glimpse core components.


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/302#issuecomment-33053845
.

Member

avanderhoorn commented Jan 22, 2014

Great feedback!

On Wed, Jan 22, 2014 at 1:47 PM, James Skimming notifications@github.comwrote:

If I, as a developer who uses Glimpse, have the ability to add custom time
line events, then it is my resposability to keep those dependencies on
Glimpse to Minimum should I desire it. If I want a trace logging convention
that results in timeline events, then I'll implement one, but I don't
expect one as part of the glimpse core components.


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/302#issuecomment-33053845
.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Jan 22, 2014

Collaborator

Alright, then we go for the approach as suggested by @nikmd23 here ?

@nikmd23 I would not combine the configuration stuff with this timing functionality in one Glimpse class, it might give naming conflicts as you mentioned but more importantly it might become a put anything we want to expose to users here kind of class. Just go for the Timeline or GlimpseTimeline approach, that way it is also easier to apply in v1 already?

Collaborator

CGijbels commented Jan 22, 2014

Alright, then we go for the approach as suggested by @nikmd23 here ?

@nikmd23 I would not combine the configuration stuff with this timing functionality in one Glimpse class, it might give naming conflicts as you mentioned but more importantly it might become a put anything we want to expose to users here kind of class. Just go for the Timeline or GlimpseTimeline approach, that way it is also easier to apply in v1 already?

Split the static Glimpse class into GlimpseConfiguration and GlimpseT…
…imeline. Refactored other classes into appropriate files/namespaces.
@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Jan 22, 2014

Member

I've refactored the code and updated the class names. Now you can use these two entry points like so:

  1. As a user, modify configuration:

    GlimpseConfiguration.Override = config => 
    {
        // modify the configuration
    };
  2. As a user, capture a timing event:

    using(GlimpseTimeline.Capture("Some chunk of code to measure"))
    {
        // some custom user code
    }
  3. As a user, capture a moment in time for the timeline:

    GlimpseTimeline.CaptureMoment("Some event I want to mark in the timeline");

A quick note about the decision to name these classes Glimpse*. Typically we try to avoid smurf naming (see #21), but I feel like it is warranted in these cases for readability purposes in end user's code. The naming convention also reinforces the fact that the user is introducing a dependency to Glimpse - valid concerns already covered by @CGijbels and @JSkimming.

There are a few places where we have smurf names but should not. One of which was IGlimpseConfiguration and the original GlimpseConfiguration (which have been renamed), and I'll do my best to clean up others on the version-2 branch.


Pending any further feedback, this branch is deemed complete and ready to be merged when the time is right.

Member

nikmd23 commented Jan 22, 2014

I've refactored the code and updated the class names. Now you can use these two entry points like so:

  1. As a user, modify configuration:

    GlimpseConfiguration.Override = config => 
    {
        // modify the configuration
    };
  2. As a user, capture a timing event:

    using(GlimpseTimeline.Capture("Some chunk of code to measure"))
    {
        // some custom user code
    }
  3. As a user, capture a moment in time for the timeline:

    GlimpseTimeline.CaptureMoment("Some event I want to mark in the timeline");

A quick note about the decision to name these classes Glimpse*. Typically we try to avoid smurf naming (see #21), but I feel like it is warranted in these cases for readability purposes in end user's code. The naming convention also reinforces the fact that the user is introducing a dependency to Glimpse - valid concerns already covered by @CGijbels and @JSkimming.

There are a few places where we have smurf names but should not. One of which was IGlimpseConfiguration and the original GlimpseConfiguration (which have been renamed), and I'll do my best to clean up others on the version-2 branch.


Pending any further feedback, this branch is deemed complete and ready to be merged when the time is right.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Jan 22, 2014

Member

Looks good to me :)

On Wed, Jan 22, 2014 at 4:27 PM, Nik Molnar notifications@github.comwrote:

I've refactored the code and updated the class names. Now you can use
these two entry points like so:

As a user, modify configuration:

GlimpseConfiguration.Override = config => {
// modify the configuration};

2.

As a user, capture a timing event:

using(GlimpseTimeline.Capture("Some chunk of code to measure"))
{
// some custom user code}

3.

As a user, capture a moment in time for the timeline:

GlimpseTimeline.CaptureMoment("Some event I want to mark in the timeline");


A quick note about the decision to name these classes Glimpse*. Typically
we try to avoid smurf naming (see #21http://www.codinghorror.com/blog/2012/07/new-programming-jargon.html),
but I feel like it is warranted in these cases for readability purposes in
end user's code. The naming convention also reinforces the fact that the
user is introducing a dependency to Glimpse - valid concerns already
covered by @CGijbels https://github.com/CGijbels and @JSkimminghttps://github.com/JSkimming
.

There are a few places where we have smurf names but should not. One of
which was IGlimpseConfiguration and the original GlimpseConfiguration(which have been renamed), and I'll do my best to clean up others on the

version-2 branch.

Pending any further feedback, this branch is deemed complete and ready to
be merged when the time is right.


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/302#issuecomment-33070191
.

Member

avanderhoorn commented Jan 22, 2014

Looks good to me :)

On Wed, Jan 22, 2014 at 4:27 PM, Nik Molnar notifications@github.comwrote:

I've refactored the code and updated the class names. Now you can use
these two entry points like so:

As a user, modify configuration:

GlimpseConfiguration.Override = config => {
// modify the configuration};

2.

As a user, capture a timing event:

using(GlimpseTimeline.Capture("Some chunk of code to measure"))
{
// some custom user code}

3.

As a user, capture a moment in time for the timeline:

GlimpseTimeline.CaptureMoment("Some event I want to mark in the timeline");


A quick note about the decision to name these classes Glimpse*. Typically
we try to avoid smurf naming (see #21http://www.codinghorror.com/blog/2012/07/new-programming-jargon.html),
but I feel like it is warranted in these cases for readability purposes in
end user's code. The naming convention also reinforces the fact that the
user is introducing a dependency to Glimpse - valid concerns already
covered by @CGijbels https://github.com/CGijbels and @JSkimminghttps://github.com/JSkimming
.

There are a few places where we have smurf names but should not. One of
which was IGlimpseConfiguration and the original GlimpseConfiguration(which have been renamed), and I'll do my best to clean up others on the

version-2 branch.

Pending any further feedback, this branch is deemed complete and ready to
be merged when the time is right.


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/302#issuecomment-33070191
.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Jan 22, 2014

Collaborator

@nikmd23 just to be clear: this branch is not supposed to be merged with master, right? Since it already contains version-2 changes?

I even wonder whether this one can still be used? And per comment of @avanderhoorn and the reply of @JSkimming there seems the need to have this feature in v1. So maybe a new PR should be created? Including the same code and then that code can be merged into version-2 through master instead of this PR?

Collaborator

CGijbels commented Jan 22, 2014

@nikmd23 just to be clear: this branch is not supposed to be merged with master, right? Since it already contains version-2 changes?

I even wonder whether this one can still be used? And per comment of @avanderhoorn and the reply of @JSkimming there seems the need to have this feature in v1. So maybe a new PR should be created? Including the same code and then that code can be merged into version-2 through master instead of this PR?

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Jan 22, 2014

Member

this branch is not supposed to be merged with master, right?

Correct, this will go into version-2, but I can't change the base branch on a PR in GitHub (or the GitHub API)

there seems the need to have this feature in v1

I agree, but it will be a completely separate implementation. I'm okay letting this wait until v2, we've gone so long without it already... But I'm not opposed to "backporting" it to v1 either assuming the API surface area remains the same.

Member

nikmd23 commented Jan 22, 2014

this branch is not supposed to be merged with master, right?

Correct, this will go into version-2, but I can't change the base branch on a PR in GitHub (or the GitHub API)

there seems the need to have this feature in v1

I agree, but it will be a completely separate implementation. I'm okay letting this wait until v2, we've gone so long without it already... But I'm not opposed to "backporting" it to v1 either assuming the API surface area remains the same.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Feb 18, 2014

Member

Like I did for "OLD - Code Based API for Configuration" #504 I'm closing this one down so that it can be resent through to the version-2 branch.

Member

avanderhoorn commented Feb 18, 2014

Like I did for "OLD - Code Based API for Configuration" #504 I'm closing this one down so that it can be resent through to the version-2 branch.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Feb 18, 2014

Member

Please comment on the decisions we need to make on the new thread for this - #745

Member

avanderhoorn commented Feb 18, 2014

Please comment on the decisions we need to make on the new thread for this - #745

@avanderhoorn avanderhoorn deleted the Custom-Timeline-Events branch Feb 19, 2014

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Mar 15, 2014

Collaborator

I wonder whether this class and corresponding IReadonlyGlimpseConfiguration shouldn't be revised. The problem lies in the readonly part of the class/interface. Because if the idea is to make sure the configuration can't be changed from the interface, then you should not expose ICollection<T> but only a IEnumerable<T> for instance, otherwise the collection can still be altered, hence the configuration, no?

I wonder whether this class and corresponding IReadonlyGlimpseConfiguration shouldn't be revised. The problem lies in the readonly part of the class/interface. Because if the idea is to make sure the configuration can't be changed from the interface, then you should not expose ICollection<T> but only a IEnumerable<T> for instance, otherwise the collection can still be altered, hence the configuration, no?

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Mar 20, 2014

Member

Yes! This was an oversight on my part. IEnumerable<T> for sure.

Member

nikmd23 replied Mar 20, 2014

Yes! This was an oversight on my part. IEnumerable<T> for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment